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

Make artifact names unique to each job in a matrix #57183

Closed
wants to merge 8 commits into from

Conversation

desrosj
Copy link
Contributor

@desrosj desrosj commented Dec 18, 2023

What?

This adds the part number to each artifact created in the End2End workflow.

Why?

After actions/upload-artifact was updated to version 4.x in #57111, the way artifacts are managed has changed. They are now uploaded on a per job basis and not per workflow. Read more on the release announcement.

The key thing to note here is that multiple jobs in the same run cannot upload to the same artifact.

It looks like all other workflows will not be affected by this because of how the matrix strategy is configured (only one job ever tries to upload an artifact with the same name).

@desrosj desrosj marked this pull request as draft December 18, 2023 20:39
@desrosj
Copy link
Contributor Author

desrosj commented Dec 18, 2023

This needs a bit more work as the artifacts are downloaded later and processed in report-to-issues.

@desrosj desrosj marked this pull request as ready for review December 19, 2023 17:02
@desrosj desrosj added the [Type] Build Tooling Issues or PRs related to build tooling label Dec 19, 2023
Copy link
Member

@kevin940726 kevin940726 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this! I think we should try merging the final report-to-issues job but then it's good to go!

@@ -127,7 +131,7 @@ jobs:
# Don't fail the job if there isn't any flaky tests report.
continue-on-error: true
with:
name: flaky-tests-report
name: flaky-tests-report-${{ matrix.part }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we can use the pattern input to download all artifacts in one step rather than splitting them into multiple parts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Good spot, I missed those new options. I think this should be addressed now.

@desrosj desrosj requested a review from kevin940726 January 5, 2024 12:27
@desrosj
Copy link
Contributor Author

desrosj commented Jan 5, 2024

@kevin940726 mind giving this another review and blessing when you have a chance?

@kevin940726
Copy link
Member

Unfortunately, there's another breaking change I didn't see mentioned in the migration guide 😅. If we use the pattern field, the step outcome will still be success even though no data is downloaded. I guess we'll need an intermediate step to determine if any report is downloaded.

This can be a follow-up though. 👍

Copy link
Member

@kevin940726 kevin940726 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@desrosj
Copy link
Contributor Author

desrosj commented Jan 8, 2024

Unfortunately, there's another breaking change I didn't see mentioned in the migration guide 😅. If we use the pattern field, the step outcome will still be success even though no data is downloaded. I guess we'll need an intermediate step to determine if any report is downloaded.

I wonder if we can use the download-path output from the action to check that an artifact was downloaded successfully. The documentation around that output is a bit scarce though. My thinking is using that in an if statement to check for an empty value, which we can use to assume nothing was downloaded.

I don't think there's any harm in adding that into this PR. I can do some experimenting later this week.

@kevin940726
Copy link
Member

kevin940726 commented Jan 9, 2024

Based on the code at: https://github.com/actions/download-artifact/blob/f44cd7b40bfd40b6aa1cc1b9b5b7bf03d3c67110/src/download-artifact.ts#L125

It will still set the output even though it downloads nothing 😆. I think the only option we have is to add a bash script to check the emptiness of the directory right after the download-artifacts step.

@WunderBart
Copy link
Member

Superseded by #61338.

@WunderBart WunderBart closed this May 7, 2024
@WunderBart WunderBart deleted the update/artifact-names-to-be-unique branch May 7, 2024 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants