Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Meta] Pull requests should be merged when they are approved #6305

Open
peternied opened this issue Jan 30, 2024 · 6 comments
Open

[Meta] Pull requests should be merged when they are approved #6305

peternied opened this issue Jan 30, 2024 · 6 comments
Assignees
Labels
2 - In progress Issue/PR: The issue or PR is in progress. discuss

Comments

@peternied
Copy link
Member

peternied commented Jan 30, 2024

Description

In GitHub when a pull request has been reviewed and approved the contributor knows it is accepted when the change has been merged. It looks like there is a practice in this repro to mark pull requests with a label 6 - Done bug waiting to merge, this is confusing. If the change is approved, then it should be merged to prevent surprise merge conflicts to the contributor that authored the change.

Here is a timeline for an outstanding pull request:

  • Oct 2nd '23 submitted
  • Nov 20 '23 approved
  • Nov 24 '23 unapproved
  • Dec 1st '23 reapproved
  • Jan 30 '24 unmerged - date of this issues creation

Because of the timescales involved is hard for a contributor to know when they are 'done' contributing.

Proposal

When a pull request is created they point to main by default; on inspection of new pull requests a maintainer determines if it should be released immediately or if not, switches the base to point to the appropriate target release branch. As soon as the pull request is approved, it is merged into the feature branch, in this case maybe 2.12 is a good name for it.

When 2.12 is released, a maintainer would make a new pull request from the 2.12 branch onto main to pull in all of those features that target the new release. This becomes the single release deployment and lets the contributors know that the maintainers of this repo will handle the deployment of the documentation changes.

@peternied
Copy link
Member Author

I couldn't find an architecture doc for this repo so its possible my suggestion doesn't align with how things are constructed. If someone could provide me with documentation, I'll be happy to revisit my recommendation to make sure its compatibly with how this project operates.

@Naarcha-AWS
Copy link
Collaborator

@peternied: When a PR is related to a new version for an unreleased distribution of OpenSearch, in this case, OpenSearch 2.12, we wait to merge the PRs until closer to release. Not only does this ensure that we aren't needing to backport already merged PRs to the new version, but it also helps the writers with tracking what pieces are done for the release and which are still in-progress.

@hdhalter hdhalter added discuss and removed untriaged labels Feb 1, 2024
@hdhalter
Copy link
Contributor

hdhalter commented Feb 1, 2024

This is a great suggestion, @peternied. We will look into what it would take to set this up in the repo.

@bbarani
Copy link
Member

bbarani commented Feb 1, 2024

I like this idea. We can enable auto-merge once all required reviews and status checks have passed. This will work only when all status checks are green though. Also, the PR should use the right set of labels.

@hdhalter
Copy link
Contributor

hdhalter commented Feb 1, 2024

We'd have to think more about auto-merge. Our current process allows for one sign-off by a maintainer technically, but our real process is tech review --> doc review --> editorial review. The editorial review is skipped if there are no wording changes. So, once the PR passes tech (by a developer) and doc review (by a maintainer), the maintainer has the responsibility of deciding whether it should go to editorial or merge. So, we'd need a checkbox that opts out of the editorial review or, if it requires editorial, requires a sign-off by the editor.

@Naarcha-AWS
Copy link
Collaborator

I could see the benefit of auto-merge for backport PRs in-particular. However, when merging to main I could see auto-merge causing issues. It frequent that, as part of the writing process, that a draft will go through several more iterations even after we've reached "Green" and "Approved" states in GitHub, for example, a release feature PR going through several technical changes before publication.

One idea @hdhalter proposed recently was cutting the branch for the future release sooner and merging release PRs into that branch. This however, would require us to update our backend pipelines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 - In progress Issue/PR: The issue or PR is in progress. discuss
Projects
None yet
Development

No branches or pull requests

4 participants