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.

Sign up for GitHub

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

Jump to bottom

[WIP] Add validation for istioctl manifest apply #21210

Closed
wants to merge 14 commits into from
Closed

[WIP] Add validation for istioctl manifest apply #21210

wants to merge 14 commits into from

Conversation

shamsher31
Copy link
Member

@shamsher31 shamsher31 commented Feb 18, 2020 •
edited by istio-policy-bot
Loading

To fix #20730

@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Feb 18, 2020
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Feb 18, 2020
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test labels Feb 18, 2020
@istio-testing
Copy link
Collaborator

Hi @shamsher31. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@shamsher31 shamsher31 changed the title Add validation for istioctl manifest apply [WIP] Add validation for istioctl manifest apply Feb 18, 2020
richardwxn
richardwxn reviewed Feb 19, 2020
View reviewed changes
Copy link
Contributor

@richardwxn richardwxn left a comment

Choose a reason for hiding this comment

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

actually we have multiple levels of validation right now, one is schema validation which is enabled now, another one is semantic and type validation, which is related to what you are doing here but somehow got disabled in a recent commit, so I am working on bringing it back, and you can probably take a look: https://github.com/istio/istio/blob/master/operator/pkg/apis/istio/v1alpha1/validation/validation.go and see how to integrate your change there

@shamsher31 shamsher31 force-pushed the set-validation branch 4 times, most recently from e869e4f to f047d19 Compare February 27, 2020 10:18
@shamsher31 shamsher31 force-pushed the set-validation branch 2 times, most recently from d316ef9 to b48cc76 Compare March 19, 2020 11:52
@shamsher31 shamsher31 mentioned this pull request Mar 19, 2020
Closed
@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Mar 21, 2020
@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 22, 2020
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 24, 2020
@shamsher31 shamsher31 force-pushed the set-validation branch 2 times, most recently from 42f4baa to f927e7c Compare March 30, 2020 12:34
@istio-testing istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 30, 2020
@istio-policy-bot
Copy link

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2020-02-19. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

@istio-policy-bot istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label Apr 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardwxn richardwxn richardwxn left review comments

Assignees
No one assigned
Labels
area/environments area/user experience cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while needs-ok-to-test size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

More validation for "istioctl install" before beginning
5 participants
@shamsher31 @istio-testing @istio-policy-bot @richardwxn @googlebot