-
Notifications
You must be signed in to change notification settings - Fork 820
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(amplify-category-api): change auth directive type and fix codegen bug #8639
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8639 +/- ##
=======================================
Coverage 56.97% 56.97%
=======================================
Files 741 741
Lines 41707 41707
Branches 8537 8537
=======================================
Hits 23764 23764
Misses 17122 17122
Partials 821 821
Continue to review full report at Codecov.
|
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.
couple nits but LGTM
@@ -15,7 +15,7 @@ export async function showSandboxModePrompts(context: $TSContext): Promise<any> | |||
printer.info( |
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.
nit: could use printer.warn
here. If you switch it, remove the leading warn
internally
for (let i = 0; i < authModes.length; i++) { | ||
const mode = authModes[i]; | ||
|
||
if (mode !== 'API_KEY' && mode !== 'AMAZON_COGNITO_USER_POOLS' && mode !== 'AWS_IAM' && mode !== 'OPENID_CONNECT' && mode !== 'AWS_LAMBDA') { | ||
if ( | ||
mode !== 'API_KEY' && | ||
mode !== 'AMAZON_COGNITO_USER_POOLS' && | ||
mode !== 'AWS_IAM' && | ||
mode !== 'OPENID_CONNECT' && | ||
mode !== 'AWS_LAMBDA' | ||
) { | ||
throw new Error(`Invalid auth mode ${mode}`); | ||
} | ||
} |
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.
nit: I know this was just a formatting change, but this could be simplified a bit to:
const allowedAuthModes = ['API_KEY', 'AWS_IAM', ...];
const invalidMode = authModes.find(mode => !allowedAuthModes.includes(mode));
if (!!invalidMode) {
throw new Error(`Invalid auth mode ${mode}`)
}
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.
If we're going to go this route, please hoist the array out of the function so that the same array can be reused. It might also make sense to use a Set instead of an array (not sure if there are enough elements to make it worth it though).
👋 Hi, this pull request was referenced in the v6.4.0 release! Check out the release notes here https://github.com/aws-amplify/amplify-cli/releases/tag/v6.4.0. |
Description of changes
changes the global authorization type to use
AuthRule
-> this also fixes the reportedamplify codegen models
bug.Description of how you validated changes
Changed unit tests, tested locally, added codegen models to the e2e tests.
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.