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

workflows: add remove-disabled-formulae workflow #67386

Merged
merged 1 commit into from
Dec 24, 2020
Merged

workflows: add remove-disabled-formulae workflow #67386

merged 1 commit into from
Dec 24, 2020

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Dec 21, 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>)?

This PR adds a workflow that will automatically remove formulae that have a disable date of more than one year ago. See discussion in #66360. I haven't really tested the actual PR-creation part of this (although I've tested the old-formula-detection part locally), so if it seems okay I'd suggest merging and seeing what happens.

@Rylan12 Rylan12 requested a review from MikeMcQuaid as a code owner December 21, 2020 21:31
@Rylan12 Rylan12 added the CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI. label Dec 21, 2020
@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Dec 21, 2020
@dawidd6
Copy link
Contributor

dawidd6 commented Dec 21, 2020

This is great and all, but...

  • I would like to see this Action added to Homebrew/actions first
  • An Action for pull request creation could be created from scratch for us, we have a bunch of workflows in Homebrew/brew that would benefit from it too
  • Use brew ruby to run the script
  • Add disable_date method to formula.rb in Homebrew/brew, as I see it isn't available now
  • Sweep all formulae to get deprecate dates via formula.rb API

I may help with some tasks if anything.

@Rylan12
Copy link
Member Author

Rylan12 commented Dec 21, 2020

Good suggestions

  • I would like to see this Action added to Homebrew/actions first

Seems fine. I didn't start by adding it there because it felt too specific to the homebrew/core repo. No problem to add it there, though.

  • An Action for pull request creation could be created from scratch for us, we have a bunch of workflows in Homebrew/brew that would benefit from it too

Why reinvent the wheel? I don't see a reason to spend time and effort creating out own when one that does the job perfectly already exists.

  • Use brew ruby to run the script

I see there's an action in the actions repo for this already (brew-script). Can this be used by another action in Homebrew/actions or should I just model it after the existing brew-script action?

  • Add disable_date method to formula.rb in Homebrew/brew, as I see it isn't available now
  • Sweep all formulae to get deprecate dates via formula.rb API

Yeah, that's probably a better way to do it. I was trying to avoid making unnecessary changes but it's probably a good idea given that deprecation/disable dates are now required.

@dawidd6
Copy link
Contributor

dawidd6 commented Dec 21, 2020

Can take brew-script and repurpose it.

About the wheel reinventing, I just think we should use only official Actions or the ones that are written by ourselves, but if others agree with you then fine.

@Rylan12
Copy link
Member Author

Rylan12 commented Dec 21, 2020

I just think we should use only official Actions or the ones that are written by ourselves, but if others agree with you then fine.

The sense I have is that we don't mind using third-party actions as long as they're pinned to a specific commit. For now, I'll continue to use the third-party action but if the consensus is that having our own is helpful (or someone else writes one for Homebrew) I have no problem adjusting.

In the meantime, I'll work on this some more tonight and see if I can get the other main points addressed.

@carlocab
Copy link
Member

carlocab commented Dec 22, 2020

Re re-inventing the wheel:

It's not just about not duplicating the work that someone else has already done. It's also about avoiding the duplication of the work that someone else is going to do. Unless Homebrew has special requirements from a create-a-pull-request action, it seems like starting from scratch also entails unnecessarily shouldering the maintenance burden of that action.

MikeMcQuaid
MikeMcQuaid previously approved these changes Dec 22, 2020
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.

Unless Homebrew has special requirements from a create-a-pull-request action, it seems like starting from scratch also entails unnecessarily shouldering the maintenance burden of that action.

There's also the maintenance of "updating the pinned commit" and "monitoring the security" of relying on an upstream action.

I think for something as relatively simple as creating a pull request it'd be good to have our own action that we can use amongst the many other actions we have that do so.

I don't think this needs to block this PR, though.

@Rylan12
Copy link
Member Author

Rylan12 commented Dec 23, 2020

I've opened a PR in the brew repo in Homebrew/brew#10108 actions repo in Homebrew/actions#133. Once those are handled (in that order) this should be ready again.

@carlocab
Copy link
Member

Does this enable a final review before candidate disabled formulae are removed? Or does one need to be paying attention to workflow runs to tell whether a disabled formula has been removed?

I don't mind the lack of a final check; I just want to be clear about the consequences of this workflow for disabled formulae.

@Rylan12
Copy link
Member Author

Rylan12 commented Dec 23, 2020

No, this will simply open a PR that removes the formulae. It will still need the usual maintainer approval and all that stuff.

Similar to the sorbet: Update RBI files PRs that are automatically generated over in the brew repo.

@carlocab
Copy link
Member

Ah, I was looking at your two other PRs which lead me to the impression that it would silently delete disabled formulae. I should've been looking at this one. 😅

@Rylan12
Copy link
Member Author

Rylan12 commented Dec 24, 2020

I tested this here and it seems to work as expected. I'm going to merge and we'll see how it goes!

@Rylan12 Rylan12 merged commit 2482b82 into Homebrew:master Dec 24, 2020
@Rylan12 Rylan12 deleted the remove-disabled-formulae branch December 24, 2020 00:57
@Rylan12
Copy link
Member Author

Rylan12 commented Dec 24, 2020

Success! 🎉🥳

#67591

@carlocab carlocab mentioned this pull request Dec 24, 2020
5 tasks
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 23, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge-skip `brew pr-automerge` will skip this pull request CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants