search

kubernetes-sigs / kubectl-validate

Apache License 2.0
121 stars 31 forks source link

Ignore empty documents. #113

Closed ah8ad3 closed 2 months ago

ah8ad3 commented 2 months ago

What type of PR is this?

/kind bug

What this PR does / why we need it:

Ignore empty documents.

Which issue(s) this PR fixes:

Fixes https://github.com/kubernetes-sigs/kubectl-validate/issues/109

Special notes for your reviewer:

I am not sure ignoring would effect other things or not (we don't have any failing test), please point out if it would.

Does this PR introduce a user-facing change?

Ignore empty documents and accept them as a valid file.
k8s-ci-robot commented 2 months ago

Welcome @ah8ad3!

It looks like this is your first PR to kubernetes-sigs/kubectl-validate 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubectl-validate has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

ah8ad3 commented 2 months ago

/cc @alexzielenski

alexzielenski commented 2 months ago

Looking thru the code for this PR i see we already have logic to skip empty YAML documents, I am curious how that bug persists...

https://github.com/kubernetes-sigs/kubectl-validate/blob/8b66d4cdabca2b502a29bc4b48c6e8ea4d820dd4/pkg/cmd/validate.go#L261-L263

alexzielenski commented 2 months ago

@ah8ad3 could you add a test case for the linked issue (---) and investigate why the existing logic for this does not skip the document as expected for that case before we consider removing error checking logic

ah8ad3 commented 2 months ago

Good catch, lazy of me to not giving it enough attention. It seems that IsEmptyYamlDocument only accepts a yaml as a empty if you have some sort of comments with # in it, otherwise it react to it as a non empty file. I changed the condition of that function and added two files to testcases/manifests one with comments and one with only --- and it should work now. PTAL @alexzielenski.

ah8ad3 commented 2 months ago

Commits squashed.

k8s-ci-robot commented 2 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ah8ad3, alexzielenski

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/kubectl-validate/blob/main/OWNERS)~~ [alexzielenski] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
alexzielenski commented 2 months ago

CI is unhappy with a comment it seems

ah8ad3 commented 2 months ago

It was a odd one, my IDE didn't warn me that :|. Gofmt was not happy with , at the end of the line. It should be fixed now. @alexzielenski .

alexzielenski commented 2 months ago

/lgtm