Ignore empty documents.

[ah8ad3]

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 #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]

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. 😃

[ah8ad3]

/cc @alexzielenski

[alexzielenski]

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...

kubectl-validate/pkg/cmd/validate.go

Lines 261 to 263 in 8b66d4c

	if utils.IsEmptyYamlDocument(document) {
	errs = append(errs, nil)
	} else {

[alexzielenski]

@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]

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.

[alexzielenski]

Thanks for the test cases! Just one comment about HasPrefix.

It surprised me that a document with just --- would include --- after being run through SplitYAMLDocuments

Usually the terminator is stripped by SplitYAMLDocuments, but reading over the implementation, it looks like the terminator is included in cases where the doc is empty: e.g. ---\n yields a single document output with contents --- where instead I would expect two empty documents.:

kubectl-validate/vendor/k8s.io/apimachinery/pkg/util/yaml/decoder.go

Lines 318 to 344 in acf913d

	sep := len([]byte(separator))
	if i := bytes.Index(line, []byte(separator)); i == 0 {
	// We have a potential document terminator
	i += sep
	trimmed := strings.TrimSpace(string(line[i:]))
	// We only allow comments and spaces following the yaml doc separator, otherwise we'll return an error
	if len(trimmed) > 0 && string(trimmed[0]) != "#" {
	return nil, YAMLSyntaxError{
	err: fmt.Errorf("invalid Yaml document separator: %s", trimmed),
	}
	}
	if buffer.Len() != 0 {
	return buffer.Bytes(), nil
	}
	if err == io.EOF {
	return nil, err
	}
	}
	if err == io.EOF {
	if buffer.Len() != 0 {
	// If we're at EOF, we have a final, non-terminated line. Return it.
	return buffer.Bytes(), nil
	}
	return nil, err
	}
	buffer.Write(line)
	}

Empty docs bypass if buffer.Len() != 0 { check and instead have their separator written to buffer

Which seems like a bug to me, but unlikely to be fixed upstream.

I think this workaround is good, IsEmptyYamlDocument returns true if every line is either a document terminator or a comment.

[alexzielenski]

Could you add some godoc explaining that this function returns true for comment-only single documents, and strings with multiple documents where all docs are comment-only.

[ah8ad3]

comment-only single documents

But it'll work for files like this too:
1:

# test doc
# test doc
---
# test doc
# test doc
---
# test doc
# test doc

2:

---
---

I think gendoc should be like this:
This function validate emptiness of yaml if it only contains comment-only documents or strings with multiple documents.

If we want to make the second example invalid we should change the logic here.

[alexzielenski]

Yes those two cases are covered by the second clause of my suggestion:

Could you add some godoc explaining that this function returns true for comment-only single documents, and strings with multiple documents where all docs are comment-only.

I'm not particular about the specific wording, as long as it mentions it works on multiple document yaml files, and comments are still considered empty.

[ah8ad3]

Updated, PTAL

[ah8ad3]

Commits squashed.

[alexzielenski]

/lgtm
/approve

[k8s-ci-robot]

[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 [alexzielenski]

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

[alexzielenski]

CI is unhappy with a comment it seems

[ah8ad3]

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]

/lgtm