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

Fixes for Verify CRDs script and invalid YAMLs #2362

Merged
merged 5 commits into from
Sep 7, 2023
Merged

Fixes for Verify CRDs script and invalid YAMLs #2362

merged 5 commits into from
Sep 7, 2023

Conversation

youngnick
Copy link
Contributor

/kind cleanup
/kind test

What this PR does / why we need it:

This updates verify-crds-kind.sh with a few changes:

  • Moves the apply operation to a bash for loop to make the checks easier to read and add
  • Updates the logic so that anything that's not an allowlisted error will cause the job to fail (there were structural errors in the GRPCRoute invalid YAMLs that meant they were the wrong type of invalid, but the check didn't pick that up. This version does).
  • Runs the invalid YAML examples both with just CEL and with webhook (since the webhook will go away soon)
  • Updates the output of the job to make finding the actual error easier.
  • Removes the v1alpha2 examples that are no longer being served (gateway, gatewayclass, and httproute v1alpha2 examples). They were also invalid in the wrong way - this error case is also handled by the new script.

Does this PR introduce a user-facing change?:

NONE

@youngnick
Fix verify-crds-kind.sh to show existing errors
0f1a0dc 
Signed-off-by: Nick Young <nick@isovalent.com>
@youngnick
Remove no longer served invalid YAMLs
de5ae29 
Signed-off-by: Nick Young <nick@isovalent.com>
@youngnick
Change output for verify-crds-kind.sh
ddc68f2 
Signed-off-by: Nick Young <nick@isovalent.com>
@youngnick
Fix invalid GRPCRoute YAMLs so they are invalid in the right way
8c05428 
Signed-off-by: Nick Young <nick@isovalent.com>
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 29, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: youngnick

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 29, 2023
@youngnick
Copy link
Contributor Author

/hold

for multiple review if necessary.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2023
youngnick
youngnick commented Aug 31, 2023
View reviewed changes
hack/verify-crds-kind.sh
@@ -14,7 +14,6 @@
# See the License for the specific language governing permissions and
# limitations under the License.

set -o errexit
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to remove this so that a single kubectl command not succeeding won't terminate the script.

youngnick
youngnick commented Aug 31, 2023
View reviewed changes
hack/verify-crds-kind.sh
## with _only_ CEL validation


for CHANNEL in experimental standard; do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole block is duplicated, yes, but I didn't want to pull this into a bash function because it's more complex that way, and the webhook section has a limited lifespan.

robscott
robscott reviewed Sep 1, 2023
View reviewed changes
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @youngnick!

hack/verify-crds-kind.sh Outdated Show resolved Hide resolved
hack/invalid-examples/v1alpha2/gateway/duplicate-listeners.yaml
@@ -1,13 +0,0 @@
apiVersion: gateway.networking.k8s.io/v1alpha2
Copy link
Member

@robscott robscott Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm assuming equivalent examples already exist in v1beta1 for every example that's being removed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there are actually more examples in v1beta1, but all the ones being removed are covered already.

@youngnick youngnick mentioned this pull request Sep 4, 2023
Merged
@youngnick
Move invalid examples to channel names instead of version
b8136bc 
Signed-off-by: Nick Young <nick@isovalent.com>
@robscott
Copy link
Member

robscott commented Sep 6, 2023

Thanks @youngnick!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 6, 2023
@robscott
Copy link
Member

robscott commented Sep 6, 2023

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 6, 2023
@k8s-ci-robot k8s-ci-robot merged commit 0089b6e into kubernetes-sigs:main Sep 7, 2023
9 checks passed
@robscott robscott mentioned this pull request Sep 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robscott robscott robscott left review comments

@bowei bowei Awaiting requested review from bowei

@shaneutt shaneutt Awaiting requested review from shaneutt

Assignees

@robscott robscott

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/test lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@youngnick @k8s-ci-robot @robscott