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

Replace experimental uninstall command with uninstall command #11736

Merged
merged 9 commits into from
Aug 25, 2022

Conversation

litong01
Copy link
Contributor

Signed-off-by: Tong Li [email protected]

Please provide a description for what this PR is for.

And to help us figure out who should review this PR, please
put an X in all the areas that this PR affects.

  • Configuration Infrastructure
  • Docs
  • Installation
  • Networking
  • Performance and Scalability
  • Policies and Telemetry
  • Security
  • Test and Release
  • User Experience
  • Developer Infrastructure

@litong01 litong01 requested a review from a team as a code owner August 23, 2022 14:30
@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 23, 2022
@@ -176,7 +176,7 @@ as the old trust domain without you having to include the aliases.
$ kubectl delete authorizationpolicy service-httpbin.default.svc.cluster.local
$ kubectl delete deploy httpbin; kubectl delete service httpbin; kubectl delete serviceaccount httpbin
$ kubectl delete deploy sleep; kubectl delete service sleep; kubectl delete serviceaccount sleep
$ istioctl x uninstall --purge
$ stioctl uninstall --purge -y
Copy link
Contributor

Choose a reason for hiding this comment

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

You want istioctl, not stioctl

@@ -138,7 +138,7 @@ snip_clean_up_1() {
kubectl delete authorizationpolicy service-httpbin.default.svc.cluster.local
kubectl delete deploy httpbin; kubectl delete service httpbin; kubectl delete serviceaccount httpbin
kubectl delete deploy sleep; kubectl delete service sleep; kubectl delete serviceaccount sleep
istioctl x uninstall --purge
stioctl uninstall --purge -y
Copy link
Contributor

Choose a reason for hiding this comment

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

misspelled here as well.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Aug 24, 2022
Tong Li and others added 6 commits August 23, 2022 22:20
Replace experimental uninstall command with uninstall command

Co-authored-by: Frank Budinsky <[email protected]>
Replace experimental uninstall command with uninstall command

Co-authored-by: Frank Budinsky <[email protected]>
@litong01 litong01 force-pushed the replace-x-uninstall branch from 6274793 to 6791544 Compare August 24, 2022 02:22
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Aug 24, 2022
@ericvn
Copy link
Contributor

ericvn commented Aug 24, 2022

A couple comments.

  • We use --skip-confirmation in a lot of the earlier pages, and then slip to using -y in later .md pages. We should be consistent in the .md pages. I don't know if @frankbu has a preference. While I like skip-confirmation, I don't think a lot of users would actually want to type the whole thing so may prefer the -y, but I don't know if we prefer using the more formal in the documentation.
  • Does it make sense to simply use the --skip-confirmation/-y parameter instead of having to do the echo y? Not sure if we are testing that one gets prompted and we are entering y? Maybe we should have several of each to test both command paths?

@litong01
Copy link
Contributor Author

A couple comments.

* We use --skip-confirmation in a lot of the earlier pages, and then slip to using -y in later .md pages. We should be consistent in the .md pages. I don't know if @frankbu has a preference. While I like skip-confirmation, I don't think a lot of users would actually want to type the whole thing so may prefer the -y, but I don't know if we prefer using the more formal in the documentation.

* Does it make sense to simply use the --skip-confirmation/-y parameter instead of having to do the echo y? Not sure if we are testing that one gets prompted and we are entering y? Maybe we should have several of each to test both command paths?

@ericvn yeah, I noticed that as well. That was the inconsistency in our docs. For document, I would prefer --skip-confirmation since it clearer than -y, having both in different places may also serve a purpose of testing both options, so I do not mind they appear at different locations.

For the pipe y thing, to be honest with you, I never liked use pipe to send a y to the command, in my view, that is really a hack, I would like to change it all to -y especially in the test scripts which no one sees, but Frank and I had a long discussion about this, where he won the argument, so that is what we had. but I can go with either.

@frankbu
Copy link
Collaborator

frankbu commented Aug 24, 2022

I would like to change it all to -y especially in the test scripts which no one sees, but Frank and I had a long discussion about this, where he won the argument, so that is what we had. but I can go with either.

I think you misunderstood my point. See here for clarification.

@frankbu
Copy link
Collaborator

frankbu commented Aug 24, 2022

I don't know if @frankbu has a preference.

I agree that users will want to use the short form -y, so using it everywhere would be my preferences.

@istio-testing istio-testing merged commit 46eb244 into istio:master Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/docs size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants