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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Oct 27, 2023
Merged

Upload pages artifact with upload-artifact v4-beta #78

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e699324
Upload pages artifact with upload-artifact@v4-beta
konradpabjan Oct 26, 2023
ecdd3ed
Switch to using download@v4-beta
konradpabjan Oct 26, 2023
ef95519
Unique artifact name per job
konradpabjan Oct 26, 2023
241a975
Correct artifact name during download
konradpabjan Oct 26, 2023
eb31309
Update artifact names in tests
konradpabjan Oct 27, 2023
1228e65
Update action.yml
konradpabjan Oct 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .github/workflows/test-hosted-runners.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,13 @@ jobs:
- name: Upload Pages artifact
uses: ./
with:
name: pages-artifact-${{ matrix.os }}
path: artifact

- name: Download artifact
uses: actions/download-artifact@v3
uses: actions/download-artifact@v4-beta
with:
name: github-pages
name: pages-artifact-${{ matrix.os }}
Copy link
Contributor

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? 🤔

Copy link
Contributor Author

@konradpabjan konradpabjan Oct 27, 2023 •
edited
Loading

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

Copy link
Contributor

@JamesMGreene JamesMGreene Oct 27, 2023 •
edited
Loading

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:

https://github.com/actions/deploy-pages/blob/fa86ad3bc1471ec672d8cb66a92c22eb13390670/action.yml#L24-L27

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. 🚀

path: artifact2

- name: Extract artifact
Expand Down
7 changes: 6 additions & 1 deletion action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ inputs:
description: "Duration after which artifact will expire in days."
required: false
default: "1"
outputs:
artifact-id:
konradpabjan marked this conversation as resolved.
Show resolved Hide resolved
description: "The ID of the artifact that was uploaded."
value: ${{ steps.upload-artifact.outputs.artifact-id }}
runs:
using: composite
steps:
Expand Down Expand Up @@ -63,7 +67,8 @@ runs:
INPUT_PATH: ${{ inputs.path }}

- name: Upload artifact
uses: actions/upload-artifact@v3
id: upload-artifact
uses: actions/upload-artifact@v4-beta
with:
name: ${{ inputs.name }}
path: ${{ runner.temp }}/artifact.tar
Expand Down