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

brew pr-pull: handle label for merging without bottles #11661

Merged
merged 1 commit into from Jul 9, 2021

Conversation

nandahkrishna
Copy link
Member

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This PR adds a --no-bottle option to pr-pull and pr-publish. This can be convenient when we want to rebase and merge a PR (and ensure commits are signed) without downloading bottles.

A use case for this would include PRs such as Homebrew/homebrew-core#80302 where we didn't add the syntax only label, since some of the changes weren't autocorrected and modified logic in e.g. test blocks.

Signatures do not persist when using the GitHub UI "Rebase and merge" button, and --no-upload would still download the bottles. This option prevents downloading the bottles when we simply want to rebase and merge.

@BrewTestBot
Copy link
Member

Review period will end on 2021-07-07 at 14:10:21 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jul 6, 2021
@MikeMcQuaid
Copy link
Member

Downloading the bottles and verifying their checksums seems appropriate in this case, no?

Would be great to figure out a way to have this workflow handled by @BrewTestBot instead. Is --autosquash insufficient here?

@nandahkrishna
Copy link
Member Author

Downloading the bottles and verifying their checksums seems appropriate in this case, no?

I don't think so – unless I'm missing something, we aren't updating the bottles here (no revision bump or not necessary to have rebuilt bottles), and they're going to be discarded anyway. The only reason for running CI would be to check whether the changes, for e.g. in test blocks across multiple formulae, were correct/didn't break the test/etc.

Would be great to figure out a way to have this workflow handled by @BrewTestBot instead. Is --autosquash insufficient here?

The intention is to use this with BrewTestBot, perhaps by passing this option to the publish workflow or creating a separate "rebase and merge" workflow. This is independent of (and can be used together with) autosquash, which is concerned with ensuring one commit per formula and rewording commit messages.

BrewTestBot already handles PRs with the CI-syntax-only label just fine – there aren't any bottles, the commits are pushed as-is but signed by BrewTestBot. This option is only useful when:

  • The CI-syntax-only label isn't applied because we want to ensure tests pass with the changes.
  • The changes do not require new bottles.
  • We want the commits to be signed (i.e. have the "verified" symbol) which doesn't happen with the "rebase and merge" button in the UI.

The alternative to this so far has been to just use pr-pull with --no-upload and push the commits to homebrew-core's master branch. This still (unnecessarily) downloads the bottles.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jul 7, 2021
@BrewTestBot
Copy link
Member

Review period ended.

@nandahkrishna nandahkrishna changed the title pr-{pull,publish}: add --no-bottle option brew pr-pull: handle label for merging without bottles Jul 8, 2021
@nandahkrishna
Copy link
Member Author

nandahkrishna commented Jul 8, 2021

With the latest commit, using the CI-no-bottles label for a PR will mean that bottles aren't published by pr-publish or the workflow. Tests will be run though (since that is handled by the homebrew-core workflow, which I think doesn't need changes to accommodate this).

Edit (forgot to add this): After a brief discussion with Mike on Slack, we agreed that handling this via a PR label was a better solution as it would then be seamlessly integrated with the Publish workflow (and the scheduled publish) without extra work from maintainers (except adding a label to the PR which is far simpler than running pr-publish with an argument).

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

This is great, makes sense to me!

@nandahkrishna nandahkrishna merged commit 625d9db into Homebrew:master Jul 9, 2021
@nandahkrishna nandahkrishna deleted the merge-no-bottles branch July 9, 2021 12:20
@github-actions github-actions bot added the outdated PR was locked due to age label Aug 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants