elastic / apm-aws-lambda

A repository for the AWS Lambda extension and other lambda related tools and build scripts.
https://www.elastic.co/guide/en/apm/guide/current/monitoring-aws-lambda.html
Apache License 2.0
16 stars 27 forks source link

Update to actions/upload-artifact@v4 #538

Closed rockdaboot closed 3 months ago

rockdaboot commented 3 months ago

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/

kruskall commented 3 months ago

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

rockdaboot commented 3 months ago

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: https://github.com/elastic/apm-aws-lambda/pull/446

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

reakaleek commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

@kruskall Do you mind to approve?