Skip to content

Conversation

@chengoramazon
Copy link
Contributor

@chengoramazon chengoramazon commented Oct 8, 2024

Problem

  • Upload url would expire in a given time. The original error message is not clear for users what to do next.

Solution

  • Handle URL expiry explicitly.
  • Add a clear error message.
URLExpired

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@chengoramazon chengoramazon requested a review from a team as a code owner October 8, 2024 16:59
@github-actions
Copy link

github-actions bot commented Oct 8, 2024

This pull request modifies code in src/ but no tests were added/updated. Confirm whether tests should be added or ensure the PR description explains why tests are not required.

@github-actions
Copy link

github-actions bot commented Oct 8, 2024

This pull request implements a feature or fix, so it must include a changelog entry. See CONTRIBUTING.md#changelog for instructions.

Copy link
Contributor

@willyyhuang willyyhuang left a comment

Choose a reason for hiding this comment

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

LGTM

}

export class UploadURLExpired extends ToolkitError {
static errorName = 'UploadURLExpired'
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this make more sense to pass in to the name field, adjacent to the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is following existing pattern.

Copy link
Contributor

@nkomonen-amazon nkomonen-amazon Oct 9, 2024

Choose a reason for hiding this comment

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

Those existing uses are incorrect, they are duplicating the existing name property. If you don't want to refactor that in this PR you can follow up and we can merge this now

@nkomonen-amazon nkomonen-amazon merged commit 4edf175 into aws:master Oct 9, 2024
22 of 24 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.

4 participants