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: add check for extra command line args provided with amplify delete #4576

Merged

Conversation

nchaloult
Copy link
Contributor

Issue #, if available:
#4115

Description of changes:
Since $ amplify delete is a sensitive and dangerous command, there should be checks in place to ensure that users truly mean to execute it. $ amplify delete may be confused with $ amplify remove <category>. If $ amplify delete is executed with any additional command line arguments, then an error message should be displayed indicating that the delete command does not accept any arguments, and a suggestion should be printed that asks the user if they meant to use the remove command instead of delete.

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

@codecov
Copy link

codecov bot commented Jun 16, 2020

Codecov Report

Merging #4576 into master will increase coverage by 0.12%.
The diff coverage is 80.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4576      +/-   ##
==========================================
+ Coverage   61.57%   61.69%   +0.12%     
==========================================
  Files         333      332       -1     
  Lines       14550    14534      -16     
  Branches     2764     2929     +165     
==========================================
+ Hits         8959     8967       +8     
+ Misses       5164     5126      -38     
- Partials      427      441      +14     
Impacted Files Coverage Δ
...ejs-function-runtime-provider/src/utils/execute.ts 75.47% <50.00%> (-0.46%) ⬇️
...aphql-auth-transformer/src/ModelAuthTransformer.ts 87.96% <50.00%> (+0.10%) ⬆️
packages/amplify-cli/src/commands/delete.js 100.00% <100.00%> (ø)
...-model-plugin/src/visitors/appsync-java-visitor.ts 90.75% <100.00%> (+0.16%) ⬆️
packages/graphql-transformer-core/src/index.ts 96.77% <100.00%> (-0.20%) ⬇️
...ackages/graphql-transformer-core/src/validation.ts 97.67% <100.00%> (ø)
packages/graphql-mapping-template/src/print.ts 35.71% <0.00%> (ø)
packages/amplify-util-mock/src/storage/storage.ts 0.00% <0.00%> (ø)
...ges/amplify-util-mock/src/CFNParser/stack/index.ts 88.09% <0.00%> (ø)
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c04c28...e3b46d0. Read the comment docs.

@nchaloult nchaloult added this to Not started ⏳ in Fellowship Contribution Board Jun 16, 2020
@nchaloult nchaloult moved this from Not started ⏳ to Pending Review 👀 in Fellowship Contribution Board Jun 16, 2020
@ammarkarachi ammarkarachi self-requested a review June 22, 2020 16:12
@ammarkarachi ammarkarachi self-assigned this Jun 22, 2020
@ammarkarachi
Copy link
Contributor

@nchaloult We have a command to support our headless users amplify delete --force I believe this would break them

@nchaloult
Copy link
Contributor Author

@nchaloult We have a command to support our headless users amplify delete --force I believe this would break them

I tested this locally with $ amplify-dev delete --force and $ amplify-dev delete <any-other-argument>. The delete --force command seems to behave as expected, even after the changes in this PR. I believe that's because of the first if statement in the getConfirmation() func in packages/amplify-cli/src/extensions/amplify-helpers/delete-project.js

if (context.input.options && context.input.options.force) {
  return {
    proceed: true,
    deleteS3: true,
    deleteAmpilfyApp: true,
  };
}

@nchaloult
Copy link
Contributor Author

Are there any other situations where $ amplify delete may expect additional command line arguments?

@ammarkarachi
Copy link
Contributor

Can you write a unit test that tests this change?

@nchaloult nchaloult force-pushed the 4115-delete-cmd-sanitization branch from d56cabd to 7cdb9df Compare June 22, 2020 19:15
@nchaloult
Copy link
Contributor Author

Can you write a unit test that tests this change?

I wrote a first draft of a set of tests.

I'm concerned about the fact that the delete.run() method calls process.exit(1) if additional command line arguments are present. I did some reading about how Jest handles situations where functions that tests call try to interrupt or kill a node process, but I'm still not sure if this will be a problem when these new tests run on this project's CI pipeline. What do you think?

@nchaloult nchaloult force-pushed the 4115-delete-cmd-sanitization branch from 7cdb9df to e3b46d0 Compare June 22, 2020 21:52
@nchaloult
Copy link
Contributor Author

Addressed the concerns with process.exit(), and moved the new test file to a new directory that the CI pipeline recognizes and runs.

@ammarkarachi ammarkarachi merged commit 82d1093 into aws-amplify:master Jul 10, 2020
Fellowship Contribution Board automation moved this from Pending Review 👀 to Complete 🙌 Jul 10, 2020
ammarkarachi added a commit that referenced this pull request Jul 10, 2020
ammarkarachi pushed a commit to ammarkarachi/amplify-cli that referenced this pull request Jul 10, 2020
…ices (aws-amplify#4610)"

This reverts commit 5bee574.

Revert "feat: add check for extra command line args provided with amplify delete (aws-amplify#4576)"

This reverts commit 82d1093.

test(amplify-category-auth): fixed push test
UnleashedMind pushed a commit to UnleashedMind/amplify-cli that referenced this pull request Jul 10, 2020
ammarkarachi pushed a commit that referenced this pull request Jul 10, 2020
* feat(amplify-provider-awscloudformation): add region ca-central-1

* fix test

* Revert "Add region ca-central-1 (#4796)"

This reverts commit c8b6bd8.

* Revert "feat: add check for extra command line args provided with amplify delete (#4576)"

This reverts commit 82d1093.

* Revert "perf: fulfill promises to upload files to S3 concurrently (#4575)"

This reverts commit 96d1914.

* Revert "fix(cli): remove unnecessary stack trace log when adding services (#4610)"

This reverts commit 5bee574.

* Revert "fix test"

This reverts commit 63c3c78.

Co-authored-by: UnleashedMind <zhoweimi@186590ce137f.ant.amazon.com>
ammarkarachi added a commit that referenced this pull request Jul 27, 2020
nikhname pushed a commit to nikhname/amplify-cli that referenced this pull request Jul 27, 2020
nikhname pushed a commit to nikhname/amplify-cli that referenced this pull request Jul 27, 2020
* feat(amplify-provider-awscloudformation): add region ca-central-1

* fix test

* Revert "Add region ca-central-1 (aws-amplify#4796)"

This reverts commit af29f35.

* Revert "feat: add check for extra command line args provided with amplify delete (aws-amplify#4576)"

This reverts commit 48b27b7.

* Revert "perf: fulfill promises to upload files to S3 concurrently (aws-amplify#4575)"

This reverts commit 9fbee8a.

* Revert "fix(cli): remove unnecessary stack trace log when adding services (aws-amplify#4610)"

This reverts commit 56efb32.

* Revert "fix test"

This reverts commit 50a8073fbbdb56931191912938fb50f542197832.

Co-authored-by: UnleashedMind <zhoweimi@186590ce137f.ant.amazon.com>
sebastiancrossa pushed a commit to MLH-Fellowship/amplify-cli that referenced this pull request Aug 4, 2020
sebastiancrossa pushed a commit to MLH-Fellowship/amplify-cli that referenced this pull request Aug 4, 2020
* feat(amplify-provider-awscloudformation): add region ca-central-1

* fix test

* Revert "Add region ca-central-1 (aws-amplify#4796)"

This reverts commit af29f35.

* Revert "feat: add check for extra command line args provided with amplify delete (aws-amplify#4576)"

This reverts commit 48b27b7.

* Revert "perf: fulfill promises to upload files to S3 concurrently (aws-amplify#4575)"

This reverts commit 9fbee8a.

* Revert "fix(cli): remove unnecessary stack trace log when adding services (aws-amplify#4610)"

This reverts commit 56efb32.

* Revert "fix test"

This reverts commit 50a8073fbbdb56931191912938fb50f542197832.

Co-authored-by: UnleashedMind <zhoweimi@186590ce137f.ant.amazon.com>
sebastiancrossa pushed a commit to MLH-Fellowship/amplify-cli that referenced this pull request Aug 25, 2020
sebastiancrossa pushed a commit to MLH-Fellowship/amplify-cli that referenced this pull request Aug 25, 2020
* feat(amplify-provider-awscloudformation): add region ca-central-1

* fix test

* Revert "Add region ca-central-1 (aws-amplify#4796)"

This reverts commit af29f35.

* Revert "feat: add check for extra command line args provided with amplify delete (aws-amplify#4576)"

This reverts commit 48b27b7.

* Revert "perf: fulfill promises to upload files to S3 concurrently (aws-amplify#4575)"

This reverts commit 9fbee8a.

* Revert "fix(cli): remove unnecessary stack trace log when adding services (aws-amplify#4610)"

This reverts commit 56efb32.

* Revert "fix test"

This reverts commit 50a8073fbbdb56931191912938fb50f542197832.

Co-authored-by: UnleashedMind <zhoweimi@186590ce137f.ant.amazon.com>
@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants