Skip to content

Conversation

esnible
Copy link
Contributor

@esnible esnible commented Jun 4, 2018

istioctl get should include an AGE column similar to kubectl.

@esnible esnible requested review from ayj and ijsnellf June 4, 2018 20:00
@ayj
Copy link
Contributor

ayj commented Jun 4, 2018

General question: There's been quite a few PR's recently making istioctl behave like kubectl. Why not use kubectl directly?

@esnible
Copy link
Contributor Author

esnible commented Jun 4, 2018

@ayj My original inspiration for istioctl get improvements was so that there could be calculated custom fields summarizing deployment configuration. See #5538

I implemented the custom fields, but to make the PRs small and manageable I broke the implementation into several PRs and the last and most interesting one is not yet delivered.

I believe there is some easy low-hanging fruit with istioctl improvements. Today I spent 90 minutes debugging a clients 404 issues. Turns out an Istio VirtualService had been modified with kubectl edit bypassing Istio validation. After a bogus change was made kubectl get virtualservice showed no apparently problem, but istioctl get virtualservice showed

json: cannot unmarshal object into Go value of type []json.RawMessage

kubectl can't do any validation; at least with istioctl we now know something is wrong.

My hope is that a few short changes to istioctl will help medium-skilled operators from shooting themselves in the foot. (I realize the particular validation problem described here is better fixed by #1894 )

@ayj
Copy link
Contributor

ayj commented Jun 5, 2018

Thanks for taking time to improve the overall user experience. My comment was more in reference to issues (I mistakenly said PRs) that have suggested feature enhancements to bring istioctl in-line with kubectl, e.g.

kubectl can't do any validation; at least with istioctl we now know something is wrong.

Server-side validation (#1894) will be enabled by default in 1.0 at which point we should remove client-side validation from istioctl. Server-side validation is also available in 0.8 but disabled by default.

Copy link
Contributor

@ayj ayj left a comment

Choose a reason for hiding this comment

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

/lgtm

@codecov
Copy link

codecov bot commented Jun 7, 2018

Codecov Report

Merging #6013 into master will increase coverage by 1%.
The diff coverage is 64%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #6013   +/-   ##
======================================
+ Coverage      67%     67%   +1%     
======================================
  Files         345     345           
  Lines       30684   30611   -73     
======================================
- Hits        20452   20444    -8     
+ Misses       9397    9330   -67     
- Partials      835     837    +2
Impacted Files Coverage Δ
pilot/pkg/model/config.go 52% <ø> (ø) ⬆️
pilot/pkg/config/kube/crd/conversion.go 88% <100%> (+1%) ⬆️
istioctl/cmd/istioctl/main.go 43% <53%> (ø) ⬇️
galley/pkg/resource/accessor.go 92% <0%> (-2%) ⬇️
mixer/adapter/stackdriver/metric/metric.go 87% <0%> (-1%) ⬇️
mixer/adapter/statsd/statsd.go 95% <0%> (ø) ⬇️
mixer/adapter/cloudwatch/handler.go 87% <0%> (ø) ⬇️
mixer/adapter/denier/denier.go 100% <0%> (ø) ⬆️
mixer/adapter/memquota/rollingWindow.go 100% <0%> (ø) ⬆️
mixer/adapter/prometheus/prometheus.go 100% <0%> (ø) ⬆️
... and 11 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 dfd9364...1c44d75. Read the comment docs.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 10, 2018
@esnible
Copy link
Contributor Author

esnible commented Jun 12, 2018

Closing; I will create a new PR after I find a fool-proof way to add a dependency.

@esnible esnible closed this Jun 12, 2018
@esnible esnible deleted the istioctl_age branch June 12, 2018 12:33
@esnible esnible restored the istioctl_age branch June 13, 2018 14:05
@esnible esnible reopened this Jun 13, 2018
@esnible esnible changed the base branch from master to release-0.8 June 13, 2018 14:08
@esnible esnible changed the base branch from release-0.8 to master June 13, 2018 14:08
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 13, 2018
@esnible
Copy link
Contributor Author

esnible commented Jun 13, 2018

Marking this WIP because I suspect the failure in ci/circleci: e2e-pilot-auth-v1alpha3-v2 is real. I introduce a new field, model.ConfigMeta.CreationTimestamp and I suspect that some code compares ConfigMeta with DeepEqual() and the timestamps cause the comparisons that previously returned true to become false.

@esnible esnible changed the title AGE column for istioctl [WIP] AGE column for istioctl Jun 13, 2018
@istio-testing istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. needs-rebase Indicates a PR needs to be rebased before being merged labels Jun 13, 2018
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 14, 2018
@esnible esnible changed the title [WIP] AGE column for istioctl AGE column for istioctl Jun 14, 2018
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jun 14, 2018
@esnible esnible changed the title AGE column for istioctl [WIP] AGE column for istioctl Jun 14, 2018
@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jun 14, 2018
@esnible esnible changed the title [WIP] AGE column for istioctl AGE column for istioctl Jun 14, 2018
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jun 14, 2018
Copy link
Contributor

@ayj ayj left a comment

Choose a reason for hiding this comment

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

/lgtm

@istio-testing
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ayj, esnible

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@istio-testing
Copy link
Collaborator

istio-testing commented Jun 14, 2018

@esnible: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/istio-pilot-e2e.sh 1c44d75 link /test istio-pilot-e2e

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@istio-testing istio-testing merged commit d915e63 into istio:master Jun 14, 2018
@esnible esnible deleted the istioctl_age branch January 29, 2019 14:47
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.

5 participants