-
Notifications
You must be signed in to change notification settings - Fork 816
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
feat: pre-deploy pull, new login mechanism and pkg cli updates #5941
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a bunch of comments, some questions.
In general these concernts are valid for the whole change:
- appId === undefined is not handled anywhere when checking for admin app and such, neither for
isAmplifyAdminApp
ordoAdminCredentialsExist
. - what is the use case when customer has 25 apps?
There are no tests yet, no unit tests or e2e.
...ategory-api/src/provider-utils/awscloudformation/service-walkthroughs/appSync-walkthrough.ts
Outdated
Show resolved
Hide resolved
...ategory-api/src/provider-utils/awscloudformation/service-walkthroughs/appSync-walkthrough.ts
Show resolved
Hide resolved
packages/amplify-provider-awscloudformation/src/utils/admin-login-server.ts
Outdated
Show resolved
Hide resolved
packages/amplify-provider-awscloudformation/src/utils/admin-login-server.ts
Outdated
Show resolved
Hide resolved
packages/amplify-provider-awscloudformation/src/utils/admin-login-server.ts
Outdated
Show resolved
Hide resolved
This pull request introduces 1 alert when merging 8fd5bdc into eb4f23d - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 770f53c into eb4f23d - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 5bb6734 into 7d48378 - view on LGTM.com new alerts:
|
Co-authored-by: Diego Costantino <diego@diegocostantino.com>
resolved concerned in the conversations above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs. Looking for a help forum? We recommend joining the Amplify Community Discord server |
Changes to support pre-deploy pull, authenticate with the browser and fixes / optimizations to the packaged CLI
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.