Skip to content

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

Merged
merged 82 commits into from
Mar 25, 2025
Merged

Seed #2546

merged 82 commits into from
Mar 25, 2025

Conversation

ShadowCat567
Copy link
Contributor

@ShadowCat567 ShadowCat567 commented Feb 27, 2025

Changes

Implements seeding capabilities for sandbox. Customers will be able to:

  1. create and run a seed script
  2. generate a policy template with additional permissions (scoped to their sandbox) needed to run seed
  3. seed their auth, data, and storage resources in sandbox
  4. securely store and use credentials for their users

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

  • 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.

Copy link

changeset-bot bot commented Feb 27, 2025

🦋 Changeset detected

Latest commit: 2bf0970

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

This PR includes changesets to release 2 packages
Name Type
@aws-amplify/seed Major
@aws-amplify/backend-cli Minor

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

@ShadowCat567 ShadowCat567 added the run-e2e Label that will include e2e tests in PR checks workflow label Feb 27, 2025
message: `Please input one-time password from EMAIL for ${user.username}:`,
});
} else {
assert.strictEqual(user.mfaPreference, 'EMAIL');
Copy link
Contributor Author

@ShadowCat567 ShadowCat567 Feb 27, 2025

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

Copy link
Member

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: {
Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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)

Copy link
Member

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.

sobolk
sobolk previously approved these changes Mar 21, 2025
Vieltojarvi and others added 2 commits March 21, 2025 15:51
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
sobolk
sobolk previously approved these changes Mar 21, 2025
Comment on lines 6 to 7
// @ts-ignore this file will not exist until sandbox is created
// import outputs from '../../amplify_outputs.json';
Copy link
Member

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 ?

sobolk
sobolk previously approved these changes Mar 24, 2025
@ShadowCat567 ShadowCat567 enabled auto-merge (squash) March 24, 2025 21:48
@ShadowCat567 ShadowCat567 disabled auto-merge March 24, 2025 21:49
Amplifiyer
Amplifiyer previously approved these changes Mar 24, 2025
…mmand.ts

Co-authored-by: Amplifiyer <51211245+Amplifiyer@users.noreply.github.com>
@ShadowCat567 ShadowCat567 dismissed stale reviews from Amplifiyer and sobolk via b7decfa March 24, 2025 23:13
@ShadowCat567 ShadowCat567 merged commit 7b9a9d2 into main Mar 25, 2025
101 of 102 checks passed
@ShadowCat567 ShadowCat567 deleted the seed-feat branch March 25, 2025 17:18
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.

3 participants