Skip to content

Improve error messages (duplicate artifacts; too many artifacts) #1600

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

Jump to bottom
Merged
merged 10 commits into from
Dec 11, 2023
Merged

Improve error messages (duplicate artifacts; too many artifacts) #1600

merged 10 commits into from
Dec 11, 2023

Conversation

vmjoseph
Copy link
Contributor

@vmjoseph vmjoseph commented Dec 8, 2023 •
edited
Loading

Adds a few catches on vague errors:

  • 409s that are tied to Conflict errors (aka artifact cannot be uploaded due to duplicate name error) now displays a more user friendly message
  • 400s that are tied to InvalidArgument errors (aka too many artifacts for 1 job) now displays a more user friendly message

@vmjoseph vmjoseph changed the title cleaning up error messages Improve error messages (duplicate artifacts; too many artifacts) Dec 8, 2023
@vmjoseph vmjoseph marked this pull request as ready for review December 8, 2023 19:49
@vmjoseph vmjoseph requested a review from a team as a code owner December 8, 2023 19:49
bethanyj28
bethanyj28 reviewed Dec 8, 2023
View reviewed changes
bethanyj28
bethanyj28 reviewed Dec 8, 2023
View reviewed changes
Copy link
Contributor

@bethanyj28 bethanyj28 left a comment

Choose a reason for hiding this comment

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

Also an additional test here might be nice <3 https://github.com/actions/toolkit/blob/main/packages/artifact/__tests__/artifact-http-client.test.ts

Wily96
Wily96 approved these changes Dec 8, 2023
View reviewed changes
Copy link

@Wily96 Wily96 left a comment

Choose a reason for hiding this comment

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

https://github.com/actions/toolkit/blob/vmjoseph%2Fdup-artifact-names/packages%2Fartifact%2Fsrc%2Finternal%2Fshared%2Fartifact-twirp-client.ts

robherley
robherley reviewed Dec 11, 2023
View reviewed changes
robherley
robherley reviewed Dec 11, 2023
View reviewed changes
robherley
robherley approved these changes Dec 11, 2023
View reviewed changes
Copy link
Contributor

@robherley robherley left a comment

Choose a reason for hiding this comment

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

👏

@vmjoseph vmjoseph merged commit 950e171 into main Dec 11, 2023
@vmjoseph vmjoseph deleted the vmjoseph/dup-artifact-names branch December 11, 2023 16:27
@vmjoseph vmjoseph mentioned this pull request Dec 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robherley robherley robherley approved these changes

@bethanyj28 bethanyj28 Awaiting requested review from bethanyj28

+1 more reviewer

@Wily96 Wily96 Wily96 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vmjoseph @bethanyj28 @robherley @Wily96