-
Notifications
You must be signed in to change notification settings - Fork 84
Seed #2546
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
Seed #2546
Conversation
🦋 Changeset detectedLatest commit: 2bf0970 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
message: `Please input one-time password from EMAIL for ${user.username}:`, | ||
}); | ||
} else { | ||
assert.strictEqual(user.mfaPreference, 'EMAIL'); |
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.
EMAIL and SMS have an assert here to ensure they are not using the same signUpChallenge
as TOTP. I would rather not do this because mfaPreference
is an optional prop and I want to keep it that way, so I would like suggestions for alternatives to this.
The main option I was considering is having a callback called totpSetupChallenge
for TOTP only and use signUpChallenge
for EMAIL and SMS
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.
The main option I was considering is having a callback called totpSetupChallenge for TOTP only and use signUpChallenge for EMAIL and SMS
That is not a bad idea. Then if we do this should we have emailSignUpChallenge and smsSignUpChallenge ?
I'm assuming this would be in AuthSignUp
how would the type look like?
let passwordSignIn = await this.authApi.confirmSignIn({ | ||
challengeResponse: user.password, | ||
options: { | ||
userAttributes: { |
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.
there must be a better way to assign userAttributes,
I cannot do userAttributes = user.userAttributes
because of the camelcase vs snakecase
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.
See
public toScreamingSnakeCase(input: string): string { |
We don't have implementation for this case it seems, but if we were to add one that would be the place.
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.
how exactly would I be able to use an implementation of toSnakeCase
(similar to toScreamingSnakeCase
) do this?
The problem is that userAttributes
looks like this:
{
family_name,
given_name,
middle_name
nickname,
preferred_username
}
and we enforce that our variable names are in camelcase so the equivalent section looks like this in AuthSignUp.userAttributes
:
{
familyName,
givenName,
middleName
nickname,
preferredUsername
}
and because of the difference in naming convention, I have to destructure AuthSignUp.userAttributes
when doing assignment to userAttributes
-- for the short period of time before I ran lint, the assignment that I want to do was possible without destructuring (because AuthSignUp.userAttributes
also had snakecase properties)
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.
Ah I see. This is a compile time problem right?
The NamingConventions
won't be useful here then - it's runtime conversion.
Please disregard the suggestion. It seems we would need some type transformation like this https://stackoverflow.com/questions/60269936/typescript-convert-generic-object-from-snake-to-camel-case - to satisfy the compiler and the runtime conversion as well to map data. Which would make solution hard to reason about and debug. What you have right now is probably best.
packages/integration-tests/src/test-projects/seed-test-project/amplify/seed/seed.ts
Fixed
Show fixed
Hide fixed
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
// @ts-ignore this file will not exist until sandbox is created | ||
// import outputs from '../../amplify_outputs.json'; |
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.
can we remove this now ?
packages/cli/src/commands/sandbox/sandbox-seed/sandbox_seed_command.ts
Outdated
Show resolved
Hide resolved
…mmand.ts Co-authored-by: Amplifiyer <[email protected]>
Changes
Implements seeding capabilities for sandbox. Customers will be able to:
New commands:
ampx sandbox seed
ampx sandbox seed generate-policy
New APIs:
getSecret()
setSecret()
createAndSignUpUser()
signInUser()
addToUserGroup()
Corresponding docs PR, if applicable: aws-amplify/docs#8294
Validation
Added new unit tests and e2e tests as well as did manual testing
Checklist
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.