[Process Improvement] Feature branch support for more efficient development

[jainankitk]

Is your feature request related to a problem? Please describe

When multiple people are working on a feature, there can be dependency between multiple PRs and can slow down the development. Some of the options considered:

1. Having common private fork and raise draft PRs against that. We tend to lose the comments/feedback when new PR created against the main repo. Also, difficult to get maintainer feedback on private fork PR.
2. Keep merging the changes in private fork and raise big PR on main repo. BIG NO

Describe the solution you'd like

I believe something like below should address this:

1. Allow creation of feature branch on main repo tracking the main branch. Developer can create github issue and one of the maintainers can evaluate and help with that. There can be few months expiration for each branch to prevent proliferation
2. Developers can create the dependent PRs on main repo using feature branch as base branch.
3. Once the dependent-on PR gets merged, base branch in above PR can be updated from feature branch to main branch
4. Delete the feature branch once dependent PRs are merged

Related component

Other

Describe alternatives you've considered

No response

Additional context

No response

[jed326]

Thanks for the suggestion @jainankitk! I had 2 initial thoughts:

1. I think you're proposing allowing the creation of feature branches that non-maintainers can push to directly, is that right? Otherwise I don't think there's anything stopping from maintainers from doing this today. I think we will need to check with @opensearch-project/admin if this is even feasible.
2. It seems like the main problem we want to solve is "if PR B depends on PR A, how can we get started on reviewing PR B before PR A is merged". One way that I've worked around this in the past is cherry-picking the commit(s) for PR A from the fork with the dependency I am waiting on and creating PR B to the main repo including the cherry-picked change. Afterwards, you can ask reviewers to filter the PR by the commits you would like them to review, excluding the cherry-picked dependency. For example, you can view PRs like this: https://github.com/opensearch-project/OpenSearch/pull/15077/files/9b494b6135f00582b437fc2340392b70a7e07080..38b3b950d4da2bde105e355af6f6a5f75c0b3112

[dblock]

We already have feature branches, and there are many: https://github.com/opensearch-project/OpenSearch/branches/all?query=feature, likely abandoned and of need of cleanup. Feel free to codify this in DEVELOPER_GUIDE and/or request a feature branch from one of the maintainers.

That said, there's no way to give r/w access to a feature branch to someone without compromising some security. So a maintainer will need to continue merging changes into the feature branch, not sure that's acceptable.

[jainankitk]

That said, there's no way to give r/w access to a feature branch to someone without compromising some security. So a maintainer will need to continue merging changes into the feature branch, not sure that's acceptable.

I was not aware of the security concerns around write access to non-main branch. Getting maintainers to merge into the feature branch defeats the purpose IMO.

[jainankitk]

I think you're proposing allowing the creation of feature branches that non-maintainers can push to directly, is that right?

@jed326 - You're spot on here!

It seems like the main problem we want to solve is "if PR B depends on PR A, how can we get started on reviewing PR B before PR A is merged". One way that I've worked around this in the past is cherry-picking the commit(s) for PR A from the fork with the dependency I am waiting on and creating PR B to the main repo including the cherry-picked change. Afterwards, you can ask reviewers to filter the PR by the commits you would like them to review, excluding the cherry-picked dependency. For example, you can view PRs like this: https://github.com/opensearch-project/OpenSearch/pull/15077/files/9b494b6135f00582b437fc2340392b70a7e07080..38b3b950d4da2bde105e355af6f6a5f75c0b3112

Thanks for this suggestion, looks interesting. Will try to use this in the short term for some of the PRs, but I believe this starts getting much complex for bigger PRs which are iterating in parallel. Also, the maintainer cannot technically approve the PR as it is still pointing to main as the base branch in case some other maintainer accidentally merges the change.

[dblock]

That said, there's no way to give r/w access to a feature branch to someone without compromising some security. So a maintainer will need to continue merging changes into the feature branch, not sure that's acceptable.

I was not aware of the security concerns around write access to non-main branch. Getting maintainers to merge into the feature branch defeats the purpose IMO.

Exactly. The issue is that you can change CI/CD that runs in target context with access to repo secrets :(

[andrross]

There was a lot of discussion around feature branches on slack a few months ago: https://opensearch.slack.com/archives/C05L60S4UBT/p1715611794391279

My 2 cents: feature branches run counter to the principles of continuous integration and we should be careful to avoid long running development that diverges from main. In my opinion, in most cases collaborative work and code sharing can happen effectively on user forks without needing a feature branch on the main repo.

As to the problem of "if PR B depends on PR A, how can we get started on reviewing PR B before PR A is merged", if they are truly dependent on one another then I'm not sure you can effectively review PR B until decisions in PR A are finalized. If it's possible to review B while A is still in flux then it seems like they are mostly independent you it should be possible to raise PRs independently.

[jainankitk]

My 2 cents: feature branches run counter to the principles of continuous integration and we should be careful to avoid long running development that diverges from main. In my opinion, in most cases collaborative work and code sharing can happen effectively on user forks without needing a feature branch on the main repo.

@andrross - My proposal is not advocating for long running development divergent from main. The feature branch is to allow concurrent development on overlapping pieces without the slow down due to dependencies. We still continue merging to main as and when components are ready.

As to the problem of "if PR B depends on PR A, how can we get started on reviewing PR B before PR A is merged", if they are truly dependent on one another then I'm not sure you can effectively review PR B until decisions in PR A are finalized. If it's possible to review B while A is still in flux then it seems like they are mostly independent you it should be possible to raise PRs independently.

Not necessarily. The dependencies might be loosely coupled allowing the PR review for A and B go independently, but A has to be merged before B can be merged. For example - if I am adding a snapshot service, and someone else is adding createSnapshot API, the createSnapshot API just needs to delegate the creation to SnapshotService. The createSnapshot API logic can be completely independent of how the SnapshotService is implemented.

[jainankitk]

Exactly. The issue is that you can change CI/CD that runs in target context with access to repo secrets :(

@dblock - Ah!, this is constraining. I am wondering if there is a way to restrict the modification access to CI/CD for specific branches?

[msfroh]

Since we wouldn't want to merge everything from a feature branch into main in one shot anyway, I'm not entirely sure what development on a feature branch on the target repo gives you that you couldn't get from collaborating on a user fork.

If you have something big/ambiguous enough that multiple people need to work on different pieces and figure out how they fit together, a user fork lets you get through those "messy" parts. Then you can start opening smaller PRs against main that work toward a solution that you know works. (You can even point to the user fork as an example of what the end product is expected to look like.)

[dblock]

@dblock - Ah!, this is constraining. I am wondering if there is a way to restrict the modification access to CI/CD for specific branches?

AFAIK no. We have a lot of infrastructure that relies on the fact that the code merged is "trusted" to some extent at least. Unfortunately I think this renders the whole discussion moot. I propose we close this.

[jainankitk]

Okay, maybe Feature branch is the wrong term to begin with. Maybe should have called this as PR branch. Take these 2 PRs for example:

• CREATE API - Add Create QueryGroup API Logic #14680
• GET API - Add Get QueryGroup API Logic #14709

Lot of the diff in GET PR is overlapping with the CREATE PR, but cannot be accounted for until CREATE PR is merged in main. Would be much easier to review GET PR if there were branch with changes for CREATE API, which could be used as baseline for GET PR until other CREATE PR is merged.

We don't even need to provide write permissions to non-maintainers for this PR branch, so should not have any security concerns as well. Thoughts @jed326 @dblock @msfroh?

[jed326]

Without looking too deeply into the logic of the 2 PRs (for purposes of trying to keep the conversation generic), why can't the "overlapping" diff part of the 2 PRs be it's own independent PR first? That should speed up maintainer reviews as well since it would be a smaller change. For example, it looks like the WorkloadManagementPlugin plugin creation related changes could be a separate smaller PR.