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

fix: update endpoints to use custom domains in admin-helpers.ts #8495

Merged
merged 2 commits into from Nov 2, 2021

Conversation

srquinn21
Copy link
Contributor

Problem:
The AppState API Gateway default endpoints have been hardcoded in the CLI configuration which puts a direct coupling between the CLI configuration and the AppState infrastructure where changes to the API infrastructure would require the CLI to be hot patched.

Solution:
Remove the hardcoded endpoints and replace with deterministic custom domains that do not change.

Testing Done:
yarn setup-dev

Description of changes

Updates config map to use custom domains rather than hard coded API Gateway endpoints

Issue #, if available

N/A

Description of how you validated changes

  • Made code change
  • built CLI locally with yarn setup-dev
  • Created a new app in the console, enabled Admin UI
  • Ran amplify-dev pull

Checklist

  • PR description included
  • yarn test passes

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

Problem:
The AppState API Gateway default endpoints have been hardcoded in the CLI configuration which puts a direct coupling between the CLI configuration and the AppState infrastructure where changes to the API infrastructure would require the CLI to be hot patched.

Solution:
Remove the hardcoded endpoints and replace with deterministic custom domains that do not change.

Testing Done:
yarn setup-dev
@srquinn21 srquinn21 requested a review from a team as a code owner October 19, 2021 20:50
@jhockett jhockett changed the title Create admin-helpers.ts fix: update endpoints to use custom domains in admin-helpers.ts Oct 19, 2021
@lgtm-com
Copy link

lgtm-com bot commented Oct 19, 2021

This pull request introduces 5 alerts when merging 06f08ce into 7a18644 - view on LGTM.com

new alerts:

  • 5 for Overwritten property

@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2021

Codecov Report

Merging #8495 (5f3d21c) into master (08704f0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8495   +/-   ##
=======================================
  Coverage   52.75%   52.75%           
=======================================
  Files         514      514           
  Lines       25921    25921           
  Branches     5058     5058           
=======================================
  Hits        13675    13675           
  Misses      11278    11278           
  Partials      968      968           
Impacted Files Coverage Δ
...vider-awscloudformation/src/utils/admin-helpers.ts 19.60% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08704f0...5f3d21c. Read the comment docs.

Copy link
Contributor

@jhockett jhockett left a comment

Choose a reason for hiding this comment

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

@srquinn21 it looks like there's some duplicate regions in the object according to the LGTM errors.

Problem:
The AppState API Gateway default endpoints have been hardcoded in the CLI configuration which puts a direct coupling between the CLI configuration and the AppState infrastructure where changes to the API infrastructure would require the CLI to be hot patched.

Solution:
Remove the hardcoded endpoints and replace with deterministic custom domains that do not change.

Testing Done:
yarn setup-dev
@lgtm-com
Copy link

lgtm-com bot commented Nov 2, 2021

This pull request introduces 5 alerts when merging 5f3d21c into 73793b4 - view on LGTM.com

new alerts:

  • 5 for Overwritten property

@srquinn21
Copy link
Contributor Author

I'm not sure why LGTM keeps saying there are key conflicts. Looking at the raw source, there are no conflicts...

Copy link
Contributor

@jhockett jhockett left a comment

Choose a reason for hiding this comment

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

LGTM - the LGTM bot is broken I think. I've seen a similar issue on another PR

@yuth yuth merged commit 2cb2f4d into aws-amplify:master Nov 2, 2021
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.

None yet

4 participants