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

.github/workflows: add action to update tapioca definitions #8283

Merged
merged 5 commits into from Aug 21, 2020

Conversation

issyl0
Copy link
Member

@issyl0 issyl0 commented Aug 10, 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?

This is a test of the changes of #8233, but raised from a branch on the main repo, not from a fork, so that the Actions run has access to the secrets. Let's see if it works!

We're expecting to see a PR raised by BrewTestBot.

@vidusheeamoli
Copy link
Contributor

Generating split RBIs into sorbet/rbi/hidden-definitions/
fatal: ambiguous argument 'Library/Homebrew/sorbet': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
Switched to a new branch 'tapioca-update-2020-08-10'
fatal: could not read Username for 'https://github.com': No such device or address
##[error]Process completed with exit code 128.

Is this related to GITHUB_TOKEN?

@issyl0
Copy link
Member Author

issyl0 commented Aug 10, 2020

Still the same problem where we can't push even from a not-forked PR. It's trying to push over HTTPS, which requires a GitHub username and a personal access token. We've got the token, which should work fine (tho I can't see what scopes it has). I wonder if there's a way to specify the username. Or we could git remote set-url git@github.com... to force SSH. 🤔

@issyl0
Copy link
Member Author

issyl0 commented Aug 10, 2020

Oh good, at least this is a different error!

@issyl0
Copy link
Member Author

issyl0 commented Aug 10, 2020

OK, so we're getting further, it's trying to push, has the right credentials, but can't push:

Switched to a new branch 'tapioca-update-2020-08-10'
fatal: refs/remotes/origin/HEAD cannot be resolved to branch

🤯

@jonchang
Copy link
Contributor

You can't push to BrewTestBot's repository using GITHUB_TOKEN.

@jonchang
Copy link
Contributor

This existing workflow already works, so you should be able to copy what it's doing without much modification: https://github.com/Homebrew/brew/blob/master/.github/workflows/spdx.yml

@issyl0 issyl0 force-pushed the test-automate-tapioca-update branch from cf33535 to 48d97cb Compare August 10, 2020 21:42
@issyl0
Copy link
Member Author

issyl0 commented Aug 10, 2020

@jonchang We're trying to push to Homebrew/brew branch tapioca-.... We've basically copied the License workflow (the only changes are the Sorbet lines, now I've reset to what @vidusheeamoli had originally). So I'm confused as to how that works but this doesn't.

@issyl0
Copy link
Member Author

issyl0 commented Aug 11, 2020

From @Bo98 in Slack:

I see your issue - you are running the action in the context of a pull request. You cannot push to origin that way - that would be a security exploit. You must test the action another way (e.g. wait for the scheduled event).

We'll have to merge this (or preferably #8233) as-is and wait three days - which shouldn't be a problem given it's the same as the License Action and we know the commands work.

@issyl0 issyl0 closed this Aug 11, 2020
@MikeMcQuaid MikeMcQuaid reopened this Aug 11, 2020
.github/workflows/tapioca-update.yml Outdated Show resolved Hide resolved
.github/workflows/tapioca-update.yml Outdated Show resolved Hide resolved
@MikeMcQuaid MikeMcQuaid changed the title [Test for #8233] .github/workflows: add action to update tapioca definitions .github/workflows: add action to update tapioca definitions Aug 11, 2020
@MikeMcQuaid MikeMcQuaid marked this pull request as ready for review August 11, 2020 12:28
@jonchang
Copy link
Contributor

Because this new workflow is triggered on the push event, it will run on any pull requests affecting that file. You still can't push to origin from a pull request due to security reasons. So the workflow needs to be broken up into two separate steps:

  1. Do the type checking and updating of sorbet files
  2. Run git commit, git push

Step 2 needs to be restricted where the GITHUB_REF is master, so it won't try to push when running against a pull request.

@jonchang
Copy link
Contributor

I just noticed this: also need to use actions/checkout since that will automatically set the right permissions to push to origin, using the default GITHUB_TOKEN.

@dawidd6
Copy link
Member

dawidd6 commented Aug 14, 2020

actions/checkout won't work well with Homebrew/actions/setup-homebrew. I would propose using Homebrew/actions/git-try-push for pushing the branch.

@MikeMcQuaid
Copy link
Member

Step 2 needs to be restricted where the GITHUB_REF is master, so it won't try to push when running against a pull request.

This isn't quite right. It's fine to run on push on Homebrew/brew and push events aren't triggered from fork pull requests.

@issyl0 issyl0 force-pushed the test-automate-tapioca-update branch 2 times, most recently from 08fce57 to d30f615 Compare August 15, 2020 21:51
@issyl0
Copy link
Member Author

issyl0 commented Aug 15, 2020

After much head-scratching (thanks so much to everyone who offered their opinions!), this finally generated a PR! 🎉 Here it is: #8361

I had to remove the date specifier for the branch as I couldn't set it dynamically to the current date in the git-try-push params, so we should set "automatically delete HEAD branches" in this repo's settings if it's not already on (I don't have enough powers to see or set it). Then every three days it'll use a clean branch to run the Tapioca stuff and raise a PR from. I think we're good at merging things fast enough to get the PR dealt with within three days!

I'll leave this open for a bit so people can check it's not doing anything weird, or if people have ideas for improvements.

EDIT: One thing I notice is CI doesn't appear to have triggered on the above test PR.

@reitermarkus
Copy link
Member

One thing I notice is CI doesn't appear to have triggered on the above test PR.

Yeah, I don't think CI is triggered by @github-actions. We had the same problem here: Homebrew/homebrew-cask-fonts#2253 (comment)

@jonchang
Copy link
Contributor

I had to remove the date specifier for the branch as I couldn't set it dynamically to the current date in the git-try-push params, so we should set "automatically delete HEAD branches" in this repo's settings if it's not already on (I don't have enough powers to see or set it).

Does setting an output variable and using it as a substitution like ${{ steps.id.output.branch }} work?

@issyl0
Copy link
Member Author

issyl0 commented Aug 16, 2020

To summarise the current status of this:

  • Pushing works with GITHUB_TOKEN.
  • Opening a PR works with GITHUB_TOKEN.
  • PRs raised by github-actions[bot] don't trigger CI, so the PR isn't mergeable. Using HOMEBREW_GITHUB_API_TOKEN to (I assume) raise the PR as BrewTestBot doesn't authenticate properly.
  • Need to test if we can get date-specific branch names with the workaround that Jon suggested.

@dawidd6
Copy link
Member

dawidd6 commented Aug 16, 2020

I'm not sure HOMEBREW_GITHUB_API_TOKEN secret is available in this repo. Had the same issue in Homebrew/actions. Someone with higher access level would need to check and enable it for this repo if it isn't. IIRC this secret is available in org settings, but I might be wrong.

@issyl0
Copy link
Member Author

issyl0 commented Aug 16, 2020

Ohh. I thought I pulled that as an example from the License action, but it turns out that I read the line wrong and it's the other way around.

Either way, we want to raise PRs as BrewTestBot, I think, so we'll need some other token than the Actions built-in one.

@MikeMcQuaid
Copy link
Member

I had to remove the date specifier for the branch as I couldn't set it dynamically to the current date in the git-try-push params, so we should set "automatically delete HEAD branches" in this repo's settings if it's not already on (I don't have enough powers to see or set it). Then every three days it'll use a clean branch to run the Tapioca stuff and raise a PR from. I think we're good at merging things fast enough to get the PR dealt with within three days!

I think it would be nice to use some sort of unique branch identifier OR to force-push to the existing branch name if there's changes rather than failing the workflow.

I'm not sure HOMEBREW_GITHUB_API_TOKEN secret is available in this repo.

It is not. If someone can let me know what rights it needs: I can add it.

Comment on lines 31 to 35
cd "$GITHUB_WORKSPACE/Library/Homebrew"
bundle install
bundle exec tapioca sync --exclude json
bundle exec srb rbi hidden-definitions
if ! git diff --exit-code -- sorbet; then
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cd "$GITHUB_WORKSPACE/Library/Homebrew"
bundle install
bundle exec tapioca sync --exclude json
bundle exec srb rbi hidden-definitions
if ! git diff --exit-code -- sorbet; then
cd "$GITHUB_WORKSPACE/Library/Homebrew"
bundle install
bundle exec tapioca sync --exclude json
bundle exec srb rbi hidden-definitions
if ! git diff --exit-code -- sorbet; then

I presume this can all be done by #8289? If not: it'd be good to add so a single command can be run.

Copy link
Member Author

Choose a reason for hiding this comment

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

@vidusheeamoli How would you feel about integrating these Tapioca update commands into brew typecheck?

Copy link
Contributor

Choose a reason for hiding this comment

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

@issyl0 On it!

@issyl0
Copy link
Member Author

issyl0 commented Aug 20, 2020

[HOMEBREW_GITHUB_API_TOKEN] is not [set]. If someone can let me know what rights it needs: I can add it.

We need this Action to be able to use a BrewTestBot PAT to push commits to a branch of this repo and raise PRs. That is, public_repo.

@Bo98
Copy link
Member

Bo98 commented Aug 20, 2020

PRs raised by github-actions[bot] don't trigger CI

Why is this the case?

@reitermarkus
Copy link
Member

@Bo98, I think it's to avoid recursive actions. You cannot trigger actions with the default GITHUB_TOKEN.

@MikeMcQuaid
Copy link
Member

@vidusheeamoli @issyl0 Check out https://github.com/Homebrew/formulae.brew.sh/blob/master/.github/workflows/scheduled.yml and https://github.com/Homebrew/rubydoc.brew.sh/blob/master/.github/workflows/scheduled.yml. I've updated them recently to use Homebrew's actions as much as possible and they should just need use the @BrewTestBot token (as mentioned above).

vidusheeamoli and others added 4 commits August 21, 2020 14:52
- Tapioca is a companion gem to Sorbet that generates RBI files for the
  Ruby gems in a project.
- Whenever Dependabot updates a gem, Tapioca has to regenerate that gem's
  RBI files so that Sorbet can accurately assess the state of the typing
  of a codebase.
- We also must regenerate Sorbet's view of
  [things defined at runtime](https://sorbet.org/docs/rbi#the-hidden-definition-rbi):
  this is what it calls `hidden-definitions`.
- Obviously, this got tedious to do manually. So here's an Action that
  runs every three days and raises a PR if there are any changes.

Co-authored-by: Vidushee Amoli <vidushee.amoli@gmail.com>
Co-authored-by: Issy Long <me@issyl0.co.uk>
Co-authored-by: Jonathan Chang <jchang641@gmail.com>
Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>
…ot's)

- The github-actions[bot] PRs don't to trigger CI runs, and are hence
  unmergeable without manual intervention.
- This'll stop needs-response[bot] from moaning at us.
@MikeMcQuaid MikeMcQuaid self-assigned this Aug 21, 2020
@MikeMcQuaid MikeMcQuaid force-pushed the test-automate-tapioca-update branch 3 times, most recently from 57d5e67 to fdda447 Compare August 21, 2020 14:33
@MikeMcQuaid
Copy link
Member

It's working: #8420

@MikeMcQuaid MikeMcQuaid merged commit 755be7a into master Aug 21, 2020
@MikeMcQuaid MikeMcQuaid deleted the test-automate-tapioca-update branch August 21, 2020 15:34
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 16, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 16, 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

8 participants