-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Fix description for istioctl verify-install #26139
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
Conversation
|
🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test? Courtesy of your friendly test nag. |
istioctl/pkg/install/verify.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still correct without passing any param?
I'm getting this with latest master code base.
$ go run "${GOPATH}"/src/istio.io/istio/istioctl/cmd/istioctl/main.go verify-install
Error: could not load IstioOperator from cluster: control plane revision "" not found. Use --filename
exit status 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm getting the same. That's what motivated this PR. The description is outdated.
See #24068 (comment) and #23087 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see. Can we clarify the msg? Via Istio operator, does it mean server side operator or istioctl? Do we need to specify istio operator here?
|
cc @irisdingbj |
|
cc @esnible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should probably release note the removal of function.
|
It changed in 1.6 :/ |
I didn't find anything in the doc that really discussed this outside the reference docs. This also was removed in 1.6, so not sure if it's appropriate to add a release note in 1.7.
|
lgtm Yep, behaviour changed for this command in 1.6 so I will agree for this pr. But I want @esnible to confirm as he made this changes. |
|
In response to a cherrypick label: new pull request created: #26359 |
Closes #24068.