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

[Actions] Label PRs where the formula is `bottle :unneeded` #48911

Merged

Conversation

@issyl0
Copy link
Member

issyl0 commented Jan 12, 2020

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

  • The vast majority of formulae here have bottles, but there are some that we can merge through the GitHub web UI buttons.
  • Yes, we'll still check the diff of submitted formulae, but for those short on time, it might be good to know that we can review submissions and merge them on GitHub without having to be near a computer to pull the bottles.
  • This was also a nice look into how we can potentially in future auto-label other conditions for formulae (new formula, test failure, etc).
@issyl0 issyl0 self-assigned this Jan 12, 2020
@SMillerDev

This comment has been minimized.

Copy link
Member

SMillerDev commented Jan 12, 2020

Looks good to me

@fxcoudert

This comment has been minimized.

Copy link
Member

fxcoudert commented Jan 12, 2020

If I read this correctly (Ruby is not my forte), this will apply the label to any PR that has at least one bottle :unneeded. But we need it to apply to PRs where all formulas are bottle unneeded.

@issyl0

This comment has been minimized.

Copy link
Member Author

issyl0 commented Jan 12, 2020

If I read this correctly (Ruby is not my forte), this will apply the label to any PR that has at least one bottle :unneeded. But we need it to apply to PRs where all formulas are bottle unneeded.

Oops - I didn't do enough testing of all the different possibilities. 😳 I've fixed this now, and in doing so made it more extensible for us to maybe add other label checks in the future.

@issyl0 issyl0 force-pushed the issyl0:gh-actions-label-bottle-unneeded-formulae-prs branch from 69fdea2 to 75b0a93 Jan 12, 2020
.github/workflows/labels.yml Outdated Show resolved Hide resolved
@issyl0 issyl0 force-pushed the issyl0:gh-actions-label-bottle-unneeded-formulae-prs branch from 75b0a93 to fb7e528 Jan 12, 2020
- The vast majority of formulae here have bottles, but there are some
  that we can merge through the GitHub web UI buttons.
- Yes, we'll still check the diff of submitted formulae, but for those
  short on time, it might be good to know that we can review submissions
  and merge them on GitHub without having to be near a computer to pull
  the bottles.
- This was also a nice look into how we can potentially in future
  auto-label other conditions for formulae (new formula, test failure,
  etc).
@issyl0 issyl0 force-pushed the issyl0:gh-actions-label-bottle-unneeded-formulae-prs branch from fb7e528 to 0ba56ac Jan 12, 2020
@issyl0 issyl0 merged commit db38f1e into Homebrew:master Jan 12, 2020
1 check passed
1 check passed
continuous-integration/jenkins/ghprb Build finished.
Details
@issyl0 issyl0 deleted the issyl0:gh-actions-label-bottle-unneeded-formulae-prs branch Jan 12, 2020
end
end

if pr_files.count == pr_labels[:no_bottles]

This comment has been minimized.

Copy link
@bayandin

bayandin Jan 12, 2020

Member

Will this work as expected if we have deleted files/formulae in a PR? And the same question about the case when we update a formula which has a versioned analogue (when PR contains alias changing)?

This comment has been minimized.

Copy link
@issyl0

issyl0 Jan 12, 2020

Author Member

It doesn't add labels if a formula has been deleted, thanks to the File.exist? check.

This comment has been minimized.

Copy link
@issyl0

issyl0 Jan 12, 2020

Author Member

However, you raise a good point about the aliases, that was one thing I didn't try over in my testing repo. I'll give it a go and push a fix if it doesn't work!

This comment has been minimized.

Copy link
@issyl0

issyl0 Jan 12, 2020

Author Member

This action will only trigger on changes to Formula/*.rb, so I think we'll be fine for aliases? What behaviour were you expecting?

This comment has been minimized.

Copy link
@bayandin

bayandin Jan 12, 2020

Member

I'm talking about PRs like #48823 (but with bottle :unneeded), It seems pr_files.count will be 2 in this case, but pr_labels[:no_bottles] is only 1 (I haven't checked it, just assuming)

Sorry, I poorly formulated the question 🙂

This comment has been minimized.

Copy link
@issyl0

issyl0 Jan 12, 2020

Author Member

Thanks - I understand the problem now! I've fixed this in #48919.

@bayandin

This comment has been minimized.

Copy link
Member

bayandin commented Jan 12, 2020

I like it a lot, thanks @issyl0 👍

Can we add tests for add-labels.rb, just to be sure that all known cases are working fine?

PS It seems my question landed just after it was merged 😄

@issyl0

This comment has been minimized.

Copy link
Member Author

issyl0 commented Jan 12, 2020

Looks like I hit merge a little too soon. Thanks for the feedback.

Can we add tests for add-labels.rb, just to be sure that all known cases are working fine?

I'd be up for this. But where would they live? I guess a spec file in the scripts dir for people to run locally? I will probably need some help though, as I've done a lot of tests for Rails-based things before, but not much on plain Ruby scripts like this.

@bayandin

This comment has been minimized.

Copy link
Member

bayandin commented Jan 12, 2020

But where would they live? I guess a spec file in the scripts dir for people to run locally?

I was thinking about extracting DSL part into https://github.com/Homebrew/brew (or https://github.com/Homebrew/.github). This extracted part should parse PR changes and provide methods like formula_updated?, revision_bumped?, uses_bottle?, formula_deleted?, new_formula? and so on, we could test it as much as we can.
Then a script for assigning labels in homebrew-core will be dead simple.

Just a thoughts.. didn't think much about it.

issyl0 added a commit to issyl0/homebrew-core that referenced this pull request Jan 12, 2020
- Sometimes, we update a formula and its alias (as in #). In that case,
  the updated formula could have `bottle :unneeded`, but the number of
  files in the PR and the number of labels wouldn't match.
- To avoid this, only operate on files in the PR that are in the
  `Formula` dir.
- Fixes Homebrew#48911 (comment).
issyl0 added a commit to issyl0/homebrew-core that referenced this pull request Jan 12, 2020
- Sometimes, we update a formula and its alias at the same time. In that
  case, the updated formula could have `bottle :unneeded`, but the number
  of files in the PR and the number of labels wouldn't match. However we
  still want the "no bottles" label, because there's only one _formula_ in
  the PR.
- To avoid this, only operate on files in the PR that are in the
  `Formula` dir.
- Fixes Homebrew#48911 (comment).
@issyl0 issyl0 mentioned this pull request Jan 12, 2020
5 of 5 tasks complete
issyl0 added a commit to issyl0/homebrew-core that referenced this pull request Jan 12, 2020
- I forgot that GitHub doesn't support [until there was a legitimate use case
  for adding a label](Homebrew#48927),
  so the action will fail:

```
/opt/hostedtoolcache/Ruby/2.6.3/x64/lib/ruby/gems/2.6.0/gems/octokit-4.15.0/lib/octokit/response/raise_error.rb:16:in `on_complete': POST https://api.github.com/repos/Homebrew/homebrew-core/issues/48927/labels: 403 - Resource not accessible by integration // See: https://developer.github.com/v3/issues/labels/#add-labels-to-an-issue (Octokit::Forbidden)
```

- Given our PR workflow is "forked repo for everything", we can't do this until
  GitHub supports using secrets in forked PRs. It's a [requested
  feature](https://github.community/t5/GitHub-Actions/how-to-use-GITHUB-TOKEN-for-PRs-from-forks/td-p/37450).
- Maybe at some other point in the future, we can revisit this and other
  auto-labelling actions.
- I enjoyed making it nonetheless, and now I have "forked repos" to add to my
  testing for future Actions proof-of-concepts!

----

Reverts Homebrew#48911 and supersedes Homebrew#48919.
issyl0 added a commit that referenced this pull request Jan 12, 2020
…ed` (#48929)

- I forgot that GitHub doesn't support secrets in Actions from forked repos, [until there was a legitimate use case for adding a label](#48927), so the action will fail:

```
/opt/hostedtoolcache/Ruby/2.6.3/x64/lib/ruby/gems/2.6.0/gems/octokit-4.15.0/lib/octokit/response/raise_error.rb:16:in `on_complete': POST https://api.github.com/repos/Homebrew/homebrew-core/issues/48927/labels: 403 - Resource not accessible by integration // See: https://developer.github.com/v3/issues/labels/#add-labels-to-an-issue (Octokit::Forbidden)
```

- Given our PR workflow is "forked repo for everything", we can't do this until
  GitHub supports using secrets in forked PRs. It's a [requested
  feature](https://github.community/t5/GitHub-Actions/how-to-use-GITHUB-TOKEN-for-PRs-from-forks/td-p/37450).
- Maybe at some other point in the future, we can revisit this and other
  auto-labelling actions.
- I enjoyed making it nonetheless, and now I have "forked repos" to add to my
  testing for future Actions proof-of-concepts!

----

Reverts #48911 and supersedes #48919.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.