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

feat: implement .autoUlid for testing purposes #10442

Merged
merged 4 commits into from
Feb 1, 2023

Conversation

ebisbe
Copy link
Contributor

@ebisbe ebisbe commented May 19, 2022

Description of changes

Add $util.autoUlid() function for Velocity template testing purposes.

Issue #, if available

Description of how you validated changes

Check the response has a Ulid format. Currently 26 characters from the spec

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies

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

@ebisbe ebisbe requested a review from a team as a code owner May 19, 2022 14:02
@danielleadams
Copy link
Contributor

Hey @ebisbe - thanks for the contribution! Since this pull request is introducing a new dependency, we need to discuss with the team. I will keep you posted with updates.

@ebisbe
Copy link
Contributor Author

ebisbe commented Jun 27, 2022

Hi @danielleadams,

I can add it as a PeerDependency, that should work right?
Also I wanted to add the new

$util.autoKsuid() : String
Returns a 128-bit randomly generated KSUID (K-Sortable Unique Identifier) base62 encoded as a String with a length of 27.

Maybe you can discuss both cases as it would add a newer dependency as well.

edwardfoyle
edwardfoyle previously approved these changes Jul 21, 2022
Copy link
Contributor

@edwardfoyle edwardfoyle left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the contribution @ebisbe 🥳

@edwardfoyle edwardfoyle self-assigned this Jul 21, 2022
@danielleadams
Copy link
Contributor

danielleadams commented Jul 22, 2022

@ebisbe running our test suite on this and the I can approve. Thanks for your patience here! (This might need to be rebased because we changed some packages names, but lets see if it works)

UPDATE: can confirm it needs a rebase.

@ebisbe
Copy link
Contributor Author

ebisbe commented Jul 25, 2022

@danielleadams I just saw your EDIT by chance ( edits don't trigger email notifications ). Do I need to do something for the rebase?

@danielleadams
Copy link
Contributor

@ebisbe the PR needs to be rebased on dev.

@ebisbe ebisbe force-pushed the appsync-simulator/util-ulid branch from 0edf7c6 to 046d9ff Compare August 3, 2022 15:49
@ebisbe
Copy link
Contributor Author

ebisbe commented Aug 3, 2022

@danielleadams done

danielleadams
danielleadams previously approved these changes Aug 8, 2022
@danielleadams
Copy link
Contributor

Thanks @ebisbe - running our test suite on it and then this will be ready to merge.

@sdstolworthy
Copy link
Contributor

@ebisbe Following up, it looks like there are a few merge conflicts that need to be resolved. We'd love to get this merged in. Would you mind tagging one of us once you have those resolved and we'll work to get this merged?

@ebisbe ebisbe force-pushed the appsync-simulator/util-ulid branch from 046d9ff to d70c1e5 Compare December 21, 2022 14:59
@ebisbe ebisbe requested a review from a team as a code owner December 21, 2022 14:59
@ebisbe ebisbe force-pushed the appsync-simulator/util-ulid branch 2 times, most recently from 015c8ce to 046d9ff Compare December 21, 2022 15:16
@ebisbe
Copy link
Contributor Author

ebisbe commented Dec 21, 2022

@sdstolworthy Merge conflict resolved.

sdstolworthy
sdstolworthy previously approved these changes Dec 22, 2022
sobolk
sobolk previously approved these changes Dec 23, 2022
danielleadams
danielleadams previously approved these changes Jan 5, 2023
@jhockett jhockett merged commit 4cf061b into aws-amplify:dev Feb 1, 2023
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.

7 participants