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
{pre,post}-build: use actions/{upload,download}-artifact@v4 #529
Conversation
b2e9d67
to
d174d1e
Compare
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 for this!
Given the failed bottle name change: is there a concern for |
Yes, was just thinking about this too. This is a real concern and we should avoid failed bottles generated from existing runs get accidentally picked up. Prioritising |
Actually, this also brings problems to the bottle cache workflow. We could probably just choose a different pattern. I'll go with |
pre-build/action.yml
Outdated
with: | ||
name: bottles | ||
pattern: bottles{,_*} |
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.
pattern: bottles{,_*} | |
pattern: bottles_* |
Because v4 cannot download v3 artifacts anyway, according to https://github.com/actions/download-artifact/tree/v4/#v4---whats-new.
This affects dependent testing, which means some of the ongoing CI jobs will fail when this is merged. Preferably the long timeout jobs should be at dep testing (i.e., after bottles are downloaded) when merging this.
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.
pr-pull uses REST API which wraps v3 and v4 that part is probably OK at least.
But yeah, for dep testing it's perhaps best to time this for when the CI queue is empty. I can also watch out for that opportunity similarly if needed.
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.
actions/download-artifact
in the homebrew-core dispatch-* workflows will also need updating after merging
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.
Thanks again @ZhongRuoyu!
Now seems to be a good time to merge this. Long-timeout jobs have both started dep testing. |
🚢 |
Companion to Homebrew/brew#17097.
Tested in ZhongRuoyu/homebrew-test#44.
Fixes #475.