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

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

Merged
merged 6 commits into from
Oct 27, 2023

Conversation

konradpabjan
Copy link
Contributor

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.

jobs:
  build:
    runs-on: ubuntu-latest
    outputs:
      artifact-id: ${{ steps.upload-pages-artifact.outputs.artifact-id }}
    steps:
      - uses: actions/checkout@v3
      - name: Build
        uses: actions/jekyll-build-pages@v1
      - name: Upload Pages artifact
        id: upload-pages-artifact
        uses: konradpabjan/upload-pages-artifact@main

  job2:
    runs-on: ubuntu-latest
    needs: build
    steps:          
      - name: Display Artifact ID
        env:
            OUTPUT1: ${{needs.build.outputs.artifact-id}}
        run: |
          echo "Artifact ID from previous job is $OUTPUT1"

image

@konradpabjan konradpabjan marked this pull request as ready for review October 27, 2023 14:18
@konradpabjan konradpabjan requested a review from a team as a code owner October 27, 2023 14:18
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

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

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

action.yml Outdated Show resolved Hide resolved
Co-authored-by: James M. Greene <JamesMGreene@github.com>
Copy link
Contributor

@JamesMGreene JamesMGreene left a 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 }}
Copy link
Contributor

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

@JamesMGreene
Copy link
Contributor

The failing test is an acceptable flake in a new service that has been noted and will be addressed. 👍🏻

@JamesMGreene JamesMGreene merged commit 0313a19 into actions:artifacts-next Oct 27, 2023
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants