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: prep work for SMS Sandbox support #7302

Merged
merged 6 commits into from
May 13, 2021

Conversation

yuth
Copy link
Contributor

@yuth yuth commented May 11, 2021

Description of changes

SNS is going to launch SMS Sandbox experience soon. This is prep work on CLI to show a warning to customers when SMS sandbox support is released

Issue #, if available

Description of how you validated changes

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

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

@yuth yuth requested a review from attilah May 11, 2021 00:57
Copy link
Contributor

@attilah attilah left a comment

Choose a reason for hiding this comment

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

Looks good!

Added some comments about naming as requested and some other comments about functionality.

conditions: {
enabled: true,
cliVersions: '4.41',
startTime: new Date(Date.now() - ONE_DAY).toISOString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we use dynamic date for fact data instead of mocking the "current date" for the expect part in the test?

packages/amplify-cli-core/src/banner-message/index.ts Outdated Show resolved Hide resolved
packages/amplify-cli/src/index.ts Show resolved Hide resolved
packages/amplify-cli-core/package.json Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented May 11, 2021

This pull request introduces 2 alerts when merging d2788a6 into 78854eb - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@yuth yuth marked this pull request as ready for review May 11, 2021 22:35
@yuth yuth requested a review from a team as a code owner May 11, 2021 22:35
@ammarkarachi ammarkarachi merged commit d1f85d2 into aws-amplify:master May 13, 2021
@github-actions
Copy link

👋 Hi, this pull request was referenced in the v4.51.0 release!

Check out the release notes here https://github.com/aws-amplify/amplify-cli/releases/tag/v4.51.0.

@github-actions github-actions bot added the referenced-in-release Issues referenced in a published release changelog label May 14, 2021
SwaySway pushed a commit that referenced this pull request May 21, 2021
* feat: add support for banner message

* chore: add check for SMS sandbox status

* chore: address review comments

* chore: fix test

* chore: fix lgtm error

* chore: change env var name
cjihrig pushed a commit to ctjlewis/amplify-cli that referenced this pull request Jul 12, 2021
* feat: add support for banner message

* chore: add check for SMS sandbox status

* chore: address review comments

* chore: fix test

* chore: fix lgtm error

* chore: change env var name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
referenced-in-release Issues referenced in a published release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants