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

mergify: Add merge automation #70

Closed
wants to merge 1 commit into from

Conversation

russellb
Copy link
Member

@russellb russellb commented Jul 2, 2024

Add PR merge automation using Mergify.
This configuration is similar to merge automation enabled on the
instructlab/instructlab and instructlab/instructlab-bot github
repositories.

While it is still possible to merge by hand, the intent of this
automation is merge things automatically once they meet specified
criteria. This should be able to handle merges the vast majority of
the time. Some exceptions include, but aren't limited to:

  • Changes to the mergify config (these won't get auto merged)

  • Changes to fix something urgently broken

  • Dealing with various infrastructure outages

Mergify has a very flexible configuration for defining merge policy.
The configuration proposed here includes:

  • Require approvals from 2 maintainers

  • There are no outstanding review requests. For example, if someone
    requests review from a specific person, the PR will not get merged
    automatically until that review is given or the review request is
    dismissed.

  • There are no "changes requested" reviews still pending. If a past
    review requested changes, the PR will not get automatically merged
    until that person reviews again.

  • There are no prohibited labels set on the PR, including hold,
    do-not-merge, or needs-rebase.

  • Expected CI jobs are all passing. This is the most verbose part of
    the configuration, because we have to define when we expect the CI
    job and when we don't. This is based on what files changed, and the
    pattern list must stay in sync with the github workflow
    configuration.

On any given PR, you can check on Mergify's analysis of the PR against
the merge criteria by clicking the Details link next to Mergify's
line item in the list of checks - the same place where CI results show
up.

Signed-off-by: Russell Bryant [email protected]

Add PR merge automation using [Mergify](https://docs.mergify.com).
This configuration is similar to merge automation enabled on the
`instructlab/instructlab` and `instructlab/instructlab-bot` github
repositories.

While it is still possible to merge by hand, the intent of this
automation is merge things automatically once they meet specified
criteria. This should be able to handle merges the vast majority of
the time. Some exceptions include, but aren't limited to:

- Changes to the mergify config (these won't get auto merged)

- Changes to fix something urgently broken

- Dealing with various infrastructure outages

Mergify has a very flexible configuration for defining merge policy.
The configuration proposed here includes:

- Require approvals from 2 maintainers

- There are no outstanding review requests. For example, if someone
  requests review from a specific person, the PR will not get merged
  automatically until that review is given or the review request is
  dismissed.

- There are no "changes requested" reviews still pending. If a past
  review requested changes, the PR will not get automatically merged
  until that person reviews again.

- There are no prohibited labels set on the PR, including `hold`,
  `do-not-merge`, or `needs-rebase`.

- Expected CI jobs are all passing. This is the most verbose part of
  the configuration, because we have to define when we expect the CI
  job and when we don't. This is based on what files changed, and the
  pattern list must stay in sync with the github workflow
  configuration.

On any given PR, you can check on Mergify's analysis of the PR against
the merge criteria by clicking the `Details` link next to Mergify's
line item in the list of checks - the same place where CI results show
up.

Signed-off-by: Russell Bryant <[email protected]>
@mergify mergify bot added the CI/CD Affects CI/CD configuration label Jul 2, 2024
@markmc
Copy link
Contributor

markmc commented Jul 3, 2024

Looks like the same policy as the instructlab one - I'd expect the same policy against repos, so +1

@russellb russellb marked this pull request as draft July 16, 2024 13:53
@russellb
Copy link
Member Author

I'd like to hold off on this for a while. I'm leaving it up as a draft because I think it's worth revisiting at some point.

@hickeyma
Copy link
Member

I'd like to hold off on this for a while. I'm leaving it up as a draft because I think it's worth revisiting at some point.

@russellb Any reasons for hold on this?

@russellb
Copy link
Member Author

I'd like to hold off on this for a while. I'm leaving it up as a draft because I think it's worth revisiting at some point.

@russellb Any reasons for hold on this?

mostly because we haven't been following the policy proposed here (requiring 2 reviews) so I'd rather wait until the repo settles to where we're ready to use more rigid merge requirements

@nathan-weinberg
Copy link
Member

I'd be in favor of moving forward with this if possible

Copy link
Member

@nathan-weinberg nathan-weinberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E2E workflow would need to be updated based on #260

@khaledsulayman
Copy link
Member

talked with Russell offline. Closing in favor of #262

jwm4 pushed a commit to jwm4/sdg that referenced this pull request Dec 13, 2024
This came up in review on instructlab#70, so document the preferred
capitalization.

Signed-off-by: Russell Bryant <[email protected]>
jwm4 pushed a commit to jwm4/sdg that referenced this pull request Dec 13, 2024
docs: Update publish strategy to include Ollama publishing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD Affects CI/CD configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants