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: auto create PRs for license data updates #8021

Merged
merged 2 commits into from Jul 18, 2020

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Jul 17, 2020

  • 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 tests with your changes locally?

Like Dependabot, but for data/spdx.json.

Currently set up to run every 12 hours, with the commit author being BrewTestBot.

@jonchang
Copy link
Contributor

I think we should instead change brew update_license_data to not pull from HEAD, but only tagged releases. cc @lionellloh for thoughts.

@lionellloh
Copy link
Contributor

I think we should instead change brew update_license_data to not pull from HEAD, but only tagged releases. cc @lionellloh for thoughts.

Hi Jon, I think that's a good idea. Tagged releases should be more reliable than changes made to the HEAD. The change to brew update-license-data should also be fairly simple. I also think that the team maintaining the repository releases quite often.


on:
schedule:
- cron: '*/60 * * * *'
Copy link
Member

Choose a reason for hiding this comment

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

Maybe run it less frequently? Like once per 8 hours?

Copy link
Contributor

Choose a reason for hiding this comment

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

With #8023 this could conceivably be as infrequent as once a day?

Copy link
Member

@dawidd6 dawidd6 left a comment

Choose a reason for hiding this comment

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

A couple of suggestions but otherwise ok. Nice work!

@jonchang
Copy link
Contributor

Needs a brew man. Is jq available on the hosted runners by default?

@Bo98
Copy link
Member Author

Bo98 commented Jul 17, 2020

Yes, it is.

@dawidd6 dawidd6 merged commit e1fdb81 into Homebrew:master Jul 18, 2020
@dawidd6
Copy link
Member

dawidd6 commented Jul 18, 2020

Let's try it. Good job @Bo98 !

@Bo98 Bo98 deleted the spdx-workflow branch July 18, 2020 09:07
@@ -16,8 +16,8 @@ def update_license_data_args

Update SPDX license data in the Homebrew repository.
EOS
switch "--fail-if-changed",
description: "Return a failing status code if current license data's version is different from " \
switch "--fail-if-not-changed",
Copy link
Member

Choose a reason for hiding this comment

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

This makes it the opposite of brew man which seems undesirable. Can you explain why they are better being inconsistent?

Copy link
Member Author

@Bo98 Bo98 Jul 20, 2020

Choose a reason for hiding this comment

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

Because, for the purposes of the workflow, I want to make sure I am only detecting when there is a change.

If I make it fail on change then I still can't be certain it has changed as it could have failed for other reasons like the download failing, GitHub API having issues, etc.

Making it a success exit code instead means I can be sure that it has downloaded successfully and it really has detected a change.

The use case of the exit code is different here than brew man. The idea is not to fail the workflow here but rather to check the exit code and push a branch if a change is detected. Accurracy of detection is important if you want to prevent the workflow pushing new branches to the repo without any changes.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, makes sense, thanks 👍 . Given that: it might make sense to make brew man (maybe even eventually brew style --fix?) behave the same and similarly be able to auto-open PRs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly. The only thing is brew man CI failures are likely caused by the PR in question (and should be fixed in the same PR), unlike outdated spdx.json data, so any new workflow would perhaps not replace the existing CI step like we did here.

There is some argument however that we should have something that sends brew man PRs for commits to Homebrew/bundle, Homebrew/services and Homebrew/test-bot.

Copy link
Member

Choose a reason for hiding this comment

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

All true. Probably worth punting on for now, I guess. Thanks!

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 23, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 23, 2020
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

6 participants