-
Notifications
You must be signed in to change notification settings - Fork 74
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
Upload pages artifact with upload-artifact v4-beta #78
Conversation
with: | ||
name: github-pages | ||
name: pages-artifact-${{ matrix.os }} |
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.
To date, we've required the artifact name to be exactly github-pages
, but I suppose we can remove that requirement for Actions Artifacts V4 so long as we are relying on the artifact ID instead of having to search for the correct artifact. 👍🏻
Speaking of which, shouldn't actions/download-artifact@v4-beta
be taking in the artifact-id
as an input parameter? 🤔
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@v4-beta
could work with just artifact IDs however we decided against that for a number of reasons (at least for now):
- Makes it easier for users to migrate from
v3
->v4
since the behavior stays largely the same - The artifact name is easier to use and more easily distinguishable than passing around an ID so for simplicity and ease of use it is easier for now. There would be a lot more output/input passing around that can cause friction for users
With regards to the name being exactly github-pages
We actually have two potential routes we can go down
Option 1
Make actions/deploy-pages
still rely on the artifact_name
. We can use the octokit rest client similar to what we do here and make a call to the list artifacts for workflow run API with a name parameter and then we will be able to confirm it exists + have an ID and pass that into the call to make a deployment.
Pros:
- Less input/output plumbing and YAML that needs to be rewritten
- Feels a bit easier for users
Cons:
- Extra HTTP call that we will have to make
Option 2
Make actions/deploy-pages
have a required artifact_id
input and get rid of the artifact_name
. This will prevent us from having to make an extra HTTP call to the list artifacts API
Pros:
- Very explicit artifact ID usage, clear what is being passed around and used
- No extra HTTP call to get the artifact ID
Cons:
- A tiny bit more YAML changes and rewriting for users but it's honestly not much
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 for the writeup!
I'm still a bit surprised that download-artifact
doesn't at least have the option to use an artifact ID as an input parameter given that upload-artifact
is now outputting that, but I acknowledge this is all still in-flux. 🤔
Sorry, I was a bit off here in general 🤦🏻♂️ as we do allow alternative names for the artifact in actions/deploy-pages
:
We can talk through the options for deploy-pages
more in the future, though I think I am liking Option 1 offhand after all since it will still give us the opportunity to warn about the size of the artifact on the action side. Not a hard requirement but a nicety. 👍🏻 Either way, that decision need not block this PR. 🚀
Co-authored-by: James M. Greene <JamesMGreene@github.com>
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! Into the future.... 🔮
with: | ||
name: github-pages | ||
name: pages-artifact-${{ matrix.os }} |
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 for the writeup!
I'm still a bit surprised that download-artifact
doesn't at least have the option to use an artifact ID as an input parameter given that upload-artifact
is now outputting that, but I acknowledge this is all still in-flux. 🤔
Sorry, I was a bit off here in general 🤦🏻♂️ as we do allow alternative names for the artifact in actions/deploy-pages
:
We can talk through the options for deploy-pages
more in the future, though I think I am liking Option 1 offhand after all since it will still give us the opportunity to warn about the size of the artifact on the action side. Not a hard requirement but a nicety. 👍🏻 Either way, that decision need not block this PR. 🚀
The failing test is an acceptable flake in a new service that has been noted and will be addressed. 👍🏻 |
Overview
Closes https://github.com/github/actions-results-team/issues/1987
We have a
v4-beta
branch of upload-artifact that we are starting to use internally. Note that it only works for select repositories that have the necessary feature flags enabled.One of the main differences between v1-v3 upload-artifact and v4 is that the artifact will become available immediately in the UI and the artifact ID will also immediately be available as a step output. See https://github.com/actions/upload-artifact/blob/aa5cae10db2b39d79f5244f6bc5084278993a3ae/action.yml#L26-L31
We need to output the artifact ID so that we can use it later in the pages deployment flow.
Testing
See https://github.com/bbq-beets/testing-artifacts-v4/actions/runs/6658237875 for an E2E flow.
With a workflow file such as this we are able to successfully output and display the artifact ID in another job. Long term this ID will be used as input to create a deployment.