-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
dev-cmd/tap-new: various improvements #17110
Conversation
It should be `github.repository_owner` rather than `github.actor`.
1. Update `actions/cache` to `v4`. 2. Unset GitHub Packages tokens if unused. 3. Quote shell variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good once null
question answered (or removed). Thanks!
Library/Homebrew/dev-cmd/tap-new.rb
Outdated
HOMEBREW_GITHUB_PACKAGES_TOKEN: #{args.github_packages? ? "${{ github.token }}" : "null"} | ||
HOMEBREW_GITHUB_PACKAGES_USER: #{args.github_packages? ? "${{ github.repository_owner }}" : "null"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the advantage in using null
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort of similar to how fine-grained tokens are configured, I think it's preferred not to set an environment variable when it's not needed. Though, technically this should already have been restricted via the permissions
above with packages: none
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is skill going to set the variable but just to a null
value which seems weird. Nicer to make the output of both of these a single conditional perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is skill going to set the variable but just to a
null
value which seems weird.
It's actually null really, because it's interpreted as a YAML null
.
Nicer to make the output of both of these a single conditional perhaps?
ea37708 should hopefully make it better by not setting those variables at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks @ZhongRuoyu!
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?actions/upload-artifact@v4
andactions/cache@v4
.github.repository_owner
rather thangithub.actor
.