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

refactor top level cli error handling #1730

Merged
merged 6 commits into from
Jul 12, 2024
Merged

refactor top level cli error handling #1730

merged 6 commits into from
Jul 12, 2024

Conversation

Amplifiyer
Copy link
Contributor

@Amplifiyer Amplifiyer commented Jul 10, 2024

Problem

  1. Errors relates to credentials and access are currently marked as system faults.
  2. Because of how parser's error handling was done, it was always emitting a "Success" metric in addition to a failure one during errors.

Issue number, if available:

Changes

  1. Remove the yargs "middleware" error handling. Instead simply rely on try/catch to perform error handling.
  2. Update FromError method to also check for CredentialsError which can happen at any aws-sdk call site and wrap it in AmplifyUserError
  3. Update AccountIdFetcher to not fail when user doesn't have credentials, rather just return a dummy value for metrics.
  4. Cache accountId in AccountIdFetcher for subsequent sandbox deployment runs.
  5. Don't fail on failing to send metrics
  6. Catch and wrap AccessDenied exceptions from sandbox

Corresponding docs PR, if applicable:

Validation

Unit tests.

Checklist

  • If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.
  • If this PR requires a change to the Project Architecture README, I have included that update in this PR.
  • If this PR requires a docs update, I have linked to that docs PR above.
  • If this PR modifies E2E tests, makes changes to resource provisioning, or makes SDK calls, I have run the PR checks with the run-e2e label set.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Amplifiyer Amplifiyer added the run-e2e Label that will include e2e tests in PR checks workflow label Jul 10, 2024
Copy link

changeset-bot bot commented Jul 10, 2024

🦋 Changeset detected

Latest commit: 8cc4cb9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@aws-amplify/platform-core Patch
@aws-amplify/sandbox Patch
@aws-amplify/backend-cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Amplifiyer Amplifiyer marked this pull request as ready for review July 10, 2024 17:44
@Amplifiyer Amplifiyer requested review from a team as code owners July 10, 2024 17:44
0618
0618 previously approved these changes Jul 11, 2024
edwardfoyle
edwardfoyle previously approved these changes Jul 11, 2024
@Amplifiyer Amplifiyer dismissed stale reviews from edwardfoyle and 0618 via 27f2447 July 11, 2024 18:26
awsluja
awsluja previously approved these changes Jul 11, 2024
@Amplifiyer Amplifiyer merged commit 44ca7d7 into main Jul 12, 2024
52 of 53 checks passed
@Amplifiyer Amplifiyer deleted the error_1 branch July 12, 2024 15:56
fangyuwu7 pushed a commit to fangyuwu7/amplify-backend that referenced this pull request Jul 15, 2024
* refactor top level cli error handling

* PR updates

* Add cause

* update api after clean build
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-e2e Label that will include e2e tests in PR checks workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants