istioctl describe: fixes, test case, remove deprecated code by esnible · Pull Request #20169 · istio/istio

esnible · 2020-01-14T18:27:35Z

Fixes istioctl x describe recent bug
Adds an integration test for istioctl x describe
Remove deprecated istioctl get put delete create set-context

Fixes #18972
Fixes #20157

howardjohn

fixes #20160

howardjohn · 2020-01-14T19:11:16Z

tests/integration/pilot/istioctl_test.go

+	framework.NewTest(t).
+		RunParallel(func(ctx framework.TestContext) {
+
+			ctx.NewSubTest("ISTIOCTL").


Suggested change

ctx.NewSubTest("ISTIOCTL").

ctx.NewSubTest("istioctl").

actually just remove the subtest entirely

howardjohn · 2020-01-14T19:12:09Z

tests/integration/pilot/istioctl_test.go

+
+					podID, err := getPodID(a)
+					if err != nil {
+						t.Fatalf("Could not get Pod ID: %v", err)


this needs to be the same thing that was passed to the func the tests runs in (ctx in this case)

I don't understand the comment. What is the problem?

See the test failure: testing.go:864: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test

You can't use the parents t, its not a valid use of the go testing library

esnible · 2020-01-14T20:02:46Z

@howardjohn Test question. The integration test works locally, and works for the pilot integration tests, but fails for integ-new-install-k8s-tests_istio because for that test istioctl x describe believes the pods are not PERMISSIVE, but DISABLE.

https://prow.istio.io/view/gcs/istio-prow/pr-logs/pull/istio_istio/20169/integ-new-install-k8s-tests_istio/5229

I am not sure what to do. I don't want to relax the output comparison because if it changes I want this test to pick it up. I don't know how to recreate locally the options used for integ-new-install-k8s-tests_istio, or why Pilot's /debug/authenticationz?proxyID will return different values when I use it.

howardjohn · 2020-01-14T20:33:23Z

@esnible that test is just running istioctl manifest apply, not sure why it would be different? At this point (I thought, but this makes me think I am wrong) its almost entirely redundant from the pilot-k8s-test, we should probably clean it up - I am curious why its different here

ayj · 2020-01-14T20:43:19Z

LGTM overall. Will add final approval after John's test comments are addressed.

esnible · 2020-01-15T20:11:33Z

/test integ-pilot-k8s-tests_istio

esnible · 2020-01-15T20:59:23Z

@ayj I have completed the things @howardjohn asked for. Please review now.

howardjohn · 2020-01-16T17:02:03Z

This is the only reason we have a fair amount of legacy junk in Pilot, will be useful to get rid of this

howardjohn · 2020-01-17T22:34:25Z

@nmittler can you take a look? Then we can clean up the config store a bit

) * istioctl describe: fixes, test case, remove deprecated code * Remove unneeded subtest structure * Remove unnecessary change in describe * Match describe output by regexp, not string * Additional output to test the tests * Keep constructor signature * Fix compilation problem * Look for Envoy config metadata in Istio 1.5.x format * 'make format' * Ran 'gen'

esnible · 2020-01-21T16:31:50Z

@howardjohn What is needed to proceed on this?

howardjohn · 2020-01-21T16:32:26Z

@esnible I am pretty sure the code changes were merged but somehow the PR is open, so Ithink we can just close this?

howardjohn · 2020-01-21T16:33:02Z

Just verified the changes are in, so I recommend just closing

nmittler · 2020-01-21T16:37:16Z

@esnible @howardjohn do we know how these changes got committed? Maybe someone force pushed from the command-line to the master branch accidentally? I wouldn't expect that would even be possible.

howardjohn · 2020-01-21T16:38:49Z

It was actually prow that merged based on the commit log. Apparently the prow logs looked normal so the assumption it was a github issue. The same happened to my PR as well and I definitely didn't force push since I wasn't around when it happened

…

On Tue, Jan 21, 2020 at 8:37 AM Nathan Mittler ***@***.***> wrote: @esnible <https://github.com/esnible> @howardjohn <https://github.com/howardjohn> do we know how these changes got committed? Maybe someone force pushed from the command-line to the master branch accidentally? I wouldn't expect that would even be possible. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20169?email_source=notifications&email_token=AAEYGXJYZ6UH57PHCX7AI6LQ64QEFA5CNFSM4KGXRJK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJQMLAA#issuecomment-576767360>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXKTXRG3RGE2T5MO6T3Q64QEFANCNFSM4KGXRJKQ> .

fejta · 2020-01-21T19:19:39Z

Please rebase this PR. It looks like github only merged some of the commits (3 of 13)?

esnible · 2020-01-21T19:52:49Z

Closing because this is already merged!

926a1bf#diff-f4fb5eb2363cca54308accde8f31be9c is the commit.

fejta · 2020-02-18T19:30:01Z

thanks

istioctl describe: fixes, test case, remove deprecated code

da601e4

esnible added the area/user experience label Jan 14, 2020

esnible requested review from ayj and nmittler January 14, 2020 18:27

esnible requested review from a team as code owners January 14, 2020 18:27

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 14, 2020

istio-testing added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 14, 2020

howardjohn approved these changes Jan 14, 2020

View reviewed changes

howardjohn reviewed Jan 14, 2020

View reviewed changes

esnible added 5 commits January 14, 2020 20:39

Merge and address review comments

5422823

Remove unneeded subtest structure

7849eb0

Remove unnecessary change in describe

aca9f90

Match describe output by regexp, not string

5e6a8ea

Additional output to test the tests

4e726ac

esnible added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jan 15, 2020

esnible added 3 commits January 15, 2020 12:05

Keep constructor signature

6239411

Fix compilation problem

adbdd88

Look for Envoy config metadata in Istio 1.5.x format

f20754d

esnible removed the request for review from nmittler January 15, 2020 20:27

esnible removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jan 15, 2020

howardjohn approved these changes Jan 15, 2020

View reviewed changes

nmittler approved these changes Jan 17, 2020

View reviewed changes

esnible added 3 commits January 17, 2020 21:02

Merge

32bd0ae

Merge remote-tracking branch 'upstream/master' into fix-describe-v3

24a26c1

'make format'

3a26213

esnible requested review from a team January 19, 2020 15:18

esnible added the do-not-merge/hold Block automatic merging of a PR. label Jan 19, 2020

Ran 'gen'

bfddccb

esnible removed the do-not-merge/hold Block automatic merging of a PR. label Jan 19, 2020

esnible added the do-not-merge/hold Block automatic merging of a PR. label Jan 21, 2020

esnible closed this Jan 21, 2020

This was referenced Jan 21, 2020

Create istioctl describe integration test #18972

Closed

"istioctl x describe pod/svc" no longer shows destination rules and virtual service config #20157

Closed

Clean up deprecated istioctl commands #20160

Closed

Conversation

esnible commented Jan 14, 2020

Uh oh!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

howardjohn Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

howardjohn Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

howardjohn Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

esnible Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

howardjohn Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

esnible commented Jan 14, 2020

Uh oh!

howardjohn commented Jan 14, 2020

Uh oh!

ayj commented Jan 14, 2020

Uh oh!

esnible commented Jan 15, 2020

Uh oh!

esnible commented Jan 15, 2020

Uh oh!

howardjohn commented Jan 16, 2020

Uh oh!

howardjohn commented Jan 17, 2020

Uh oh!

esnible commented Jan 21, 2020

Uh oh!

howardjohn commented Jan 21, 2020

Uh oh!

howardjohn commented Jan 21, 2020

Uh oh!

nmittler commented Jan 21, 2020

Uh oh!

howardjohn commented Jan 21, 2020 via email

Uh oh!

fejta commented Jan 21, 2020

Uh oh!

esnible commented Jan 21, 2020

Uh oh!

fejta commented Feb 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants