Skip to content

Update to actions/upload-artifact@v4 #538

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

Merged
merged 2 commits into from
Aug 19, 2024

Conversation

rockdaboot
Copy link
Contributor

Fixes this warning in the GitHub CI annotations:
The following actions use a deprecated Node.js version and will be forced to run on node20: actions/upload-artifact@v3.
For more info: https://github.blog/changelog/2024-03-07-github-actions-all-actions-will-run-on-node20-instead-of-node16-by-default/

Fixes this warning in the GitHub CI annotations:
The following actions use a deprecated Node.js version and will be forced to
run on node20: actions/upload-artifact@v3.
For more info: https://github.blog/changelog/2024-03-07-github-actions-all-actions-will-run-on-node20-instead-of-node16-by-default/
@github-actions github-actions bot added the aws-λ-extension AWS Lambda Extension label Aug 13, 2024
@kruskall
Copy link
Member

This should be kept up to date by dependabot. Why didn't we get a pr ? 🤔

@rockdaboot
Copy link
Contributor Author

This should be kept up to date by dependabot. Why didn't we get a pr ? 🤔

Good point. I just searched the PRs and found:
#446

Missing reasoning why the PR was closed.
@reakaleek Do you remember?

@reakaleek
Copy link
Member

reakaleek commented Aug 19, 2024

This should be kept up to date by dependabot. Why didn't we get a pr ? 🤔

Good point. I just searched the PRs and found: #446

Missing reasoning why the PR was closed. @reakaleek Do you remember?

The actions/upload-artifact@v4 action had a breaking change. We had to skip the update in several repositories because some workflows couldn't handle the change. Also, it is/was incompatible with our custom actions.

I might have skipped it as a precaution here. We still need to migrate to v4 properly.

If v4 is working correctly in this repo, please feel free to go ahead.

@rockdaboot
Copy link
Contributor Author

rockdaboot commented Aug 19, 2024

@reakaleek Thanks for the details.
We only use @V3 in one place:

  test:
    strategy:
      fail-fast: false
      matrix:
        platform:
          - "ubuntu-latest"
          - "macos-latest"
    runs-on: ${{ matrix.platform }}
    steps:
      - uses: actions/checkout@v4
      - name: Bootstrap Action Workspace
        uses: ./.github/actions/bootstrap
      - name: Test
        run: make test junitfile="${{ matrix.platform }}-junit-report.xml"
      - uses: actions/upload-artifact@v3
        if: success() || failure()
        with:
          name: test-results-${{ matrix.platform }}
          path: '*-junit-report.xml'

To decide if updating v3 to v4 is working correctly, we need to know who the consumer of *-junit-report.xml is. Do you have any information about this?

[UPDATE] Just found this

 report:
    runs-on: ubuntu-latest
    steps:
      - uses: elastic/apm-pipeline-library/.github/actions/test-report@current
        with:
          artifact: /test-results-(.*)/
          name: 'JUnit Tests $1'
          path: "*-junit-report.xml"        # Path to test results (inside artifact .zip)
          reporter: java-junit              # Format of test results

@reakaleek
Copy link
Member

@reakaleek Thanks for the details. We only use @V3 in one place:

  test:
    strategy:
      fail-fast: false
      matrix:
        platform:
          - "ubuntu-latest"
          - "macos-latest"
    runs-on: ${{ matrix.platform }}
    steps:
      - uses: actions/checkout@v4
      - name: Bootstrap Action Workspace
        uses: ./.github/actions/bootstrap
      - name: Test
        run: make test junitfile="${{ matrix.platform }}-junit-report.xml"
      - uses: actions/upload-artifact@v3
        if: success() || failure()
        with:
          name: test-results-${{ matrix.platform }}
          path: '*-junit-report.xml'

To decide if updating v3 to v4 is working correctly, we need to know who the consumer of *-junit-report.xml is. Do you have any information about this?

[UPDATE] Just found this

 report:
    runs-on: ubuntu-latest
    steps:
      - uses: elastic/apm-pipeline-library/.github/actions/test-report@current
        with:
          artifact: /test-results-(.*)/
          name: 'JUnit Tests $1'
          path: "*-junit-report.xml"        # Path to test results (inside artifact .zip)
          reporter: java-junit              # Format of test results

@reakaleek Thanks for the details. We only use @V3 in one place:

  test:
    strategy:
      fail-fast: false
      matrix:
        platform:
          - "ubuntu-latest"
          - "macos-latest"
    runs-on: ${{ matrix.platform }}
    steps:
      - uses: actions/checkout@v4
      - name: Bootstrap Action Workspace
        uses: ./.github/actions/bootstrap
      - name: Test
        run: make test junitfile="${{ matrix.platform }}-junit-report.xml"
      - uses: actions/upload-artifact@v3
        if: success() || failure()
        with:
          name: test-results-${{ matrix.platform }}
          path: '*-junit-report.xml'

To decide if updating v3 to v4 is working correctly, we need to know who the consumer of *-junit-report.xml is. Do you have any information about this?

Ah, thank you for the context. The only consumer of the junit report is https://github.com/elastic/apm-aws-lambda/actions/workflows/test-reporter.yml, but it's disabled because it's broken for another reason, which we haven't been able to solve yet.

I think this workflow was actually the reason why we did not upgrade to v4.

In some repositories, we dismissed the test reporter workflow entirely. Hence, you can upgrade to v4.

@rockdaboot rockdaboot enabled auto-merge (squash) August 19, 2024 09:17
@rockdaboot
Copy link
Contributor Author

@kruskall Do you mind to approve?

@rockdaboot rockdaboot requested a review from kruskall August 19, 2024 09:18
@rockdaboot rockdaboot merged commit 6a4ee87 into elastic:main Aug 19, 2024
10 checks passed
@rockdaboot rockdaboot deleted the update-upload-artifact branch August 19, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws-λ-extension AWS Lambda Extension
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants