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: CLI add printout of what has been deleted #8894

Merged
merged 1 commit into from
Mar 31, 2022

Conversation

reginapizza
Copy link
Contributor

@reginapizza reginapizza commented Mar 24, 2022

Closes #8828

CLI will print out the following upon resource deletion:

$ argocd app delete guestbook
Are you sure you want to delete 'guestbook' and all its resources? [y/n]
y
application 'guestbook' deleted

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

Copy link
Member

@pasha-codefresh pasha-codefresh left a comment

Choose a reason for hiding this comment

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

Thank you, LGTM

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@danielhelfand danielhelfand left a comment

Choose a reason for hiding this comment

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

Just wanted to note that this will say things are deleted even if nothing is deleted. There should be some confirmation that the app was actually deleted before the message is displayed.

An example of this is when you try to delete an app that doesn't exist:

dist/argocd app delete foo
Are you sure you want to delete 'foo' and all its resources? [y/n]
y
application 'foo' deleted
FATA[0003] rpc error: code = NotFound desc = applications.argoproj.io "foo" not found 

This would also occur when n is input to say not to delete an app.

@danielhelfand
Copy link
Contributor

danielhelfand commented Mar 24, 2022

Maybe the deletion confirmation message should be output after here assuming the err is nil from the Delete call?

@reginapizza reginapizza force-pushed the cliDeletePrintout branch 3 times, most recently from 5b71803 to f6f6066 Compare March 28, 2022 18:45
@reginapizza
Copy link
Contributor Author

@danielhelfand thanks for catching that, let me know if what I have now is better!

@codecov
Copy link

codecov bot commented Mar 28, 2022

Codecov Report

Merging #8894 (3243807) into master (8847a31) will increase coverage by 1.51%.
The diff coverage is 63.30%.

@@            Coverage Diff             @@
##           master    #8894      +/-   ##
==========================================
+ Coverage   43.40%   44.92%   +1.51%     
==========================================
  Files         186      212      +26     
  Lines       23373    25262    +1889     
==========================================
+ Hits        10145    11348    +1203     
- Misses      11779    12310     +531     
- Partials     1449     1604     +155     
Impacted Files Coverage Δ
applicationset/services/pull_request/fake.go 0.00% <0.00%> (ø)
applicationset/utils/createOrUpdate.go 0.00% <0.00%> (ø)
applicationset/utils/policy.go 0.00% <0.00%> (ø)
cmd/argocd/commands/app.go 9.16% <0.00%> (-0.01%) ⬇️
applicationset/services/pull_request/github.go 14.00% <14.00%> (ø)
...is/applicationset/v1alpha1/applicationset_types.go 34.69% <34.69%> (ø)
applicationset/generators/scm_provider.go 53.44% <53.44%> (ø)
applicationset/generators/pull_request.go 56.25% <56.25%> (ø)
...cationset/controllers/applicationset_controller.go 56.75% <56.75%> (ø)
applicationset/services/repo_service.go 57.14% <57.14%> (ø)
... and 20 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 e864652...3243807. Read the comment docs.

@danielhelfand
Copy link
Contributor

danielhelfand commented Mar 28, 2022

@reginapizza lgtm! Thanks for making the change.

You need to rebase around master branch to fix this failed workflow run: Integration tests / Check changes to generated code (pull_request). The failure is unrelated to your change and was fixed in #8912.

Signed-off-by: Regina Scott <rescott@redhat.com>
@crenshaw-dev crenshaw-dev merged commit f1e0c84 into argoproj:master Mar 31, 2022
wojtekidd pushed a commit to wojtekidd/argo-cd that referenced this pull request Apr 25, 2022
Signed-off-by: Regina Scott <rescott@redhat.com>
Signed-off-by: wojtekidd <wojtek.cichon@protonmail.com>
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.

cli: print out what has been deleted
5 participants