-
Notifications
You must be signed in to change notification settings - Fork 34
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.
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
feat: add support for multiple objects in files #12
feat: add support for multiple objects in files #12
Conversation
could you also add a few cases case to the test suite?
testing infra may also need to be modified to parse the error comments correctly |
Will do |
Other wise, I think it looks correct, thanks. |
But I think some test are not passing currently. |
@eddycharly Is there anything I can do to help unblock this? |
@alexzielenski there's a bug in this implementation and i need to add some tests, will do right now. |
066717e
to
e6f295a
Compare
pkg/cmd/validate.go
Outdated
onlyComments := true | ||
for _, line := range strings.Split(string(document), "\n") { | ||
if strings.TrimSpace(line) == "" { | ||
continue | ||
} else if !strings.HasPrefix(line, "#") { | ||
onlyComments = false | ||
break | ||
} | ||
} | ||
if !onlyComments { | ||
documents = append(documents, []byte(document)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apelisse @alexzielenski does this make sense ? (skip the document if it contains only comments)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that mirror's kubectl's behavior. Though I think if the list of documents at the end is empty there should be an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a per file basis ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on a per-invocation basis. Mirroring kubectl's behavior
kubectl-validate ./emptyfile ./pod.yaml
is accepted and validates pod.yaml
kubectl-validate ./emptyfile
is an error since there were no documents given to validate
kubectl-validate ./emptyfile ./anotheremptyfile
is an error since there were no documents given to validate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, this means we have to load all files first then check for potential error and then eventually start processing.
it changes the logic we have today where we process one file at a time.
can we log an issue and address this in another PR ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm
Added more test cases |
1dbbbe1
to
58a18e8
Compare
fmt.Fprintln(cmd.OutOrStdout(), string(data)) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func ValidateFile(filePath string, resolver *validatorfactory.ValidatorFactory) error { | ||
func ValidateFile(filePath string, resolver *validatorfactory.ValidatorFactory) []error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexzielenski is this a good approach to return a slice of errors ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but I think the skipped documents should leave nil
error element so the returned errors matches and ordering and amount of input documents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the nil
error will result in a success Status
entry, looks strange as it didn't exercise the validation process at all (a skipped document is one that contains only comments right ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is true. I am concerned about the confusing interface where input documents != output statuses. What do you think of this idea:
Rather than return a slice of errors, return a map of GVKNN (group-verison-kind-namespace-name) to error. This would make it very clear which error is associated with which document. Skipped documents would not need to be included. The human output would remain the same, but the JSON output contains clear machine-readable association between document and error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that but it is allowed to have multiple identical GVKNN documents in the same file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rats. Tough. I am still learning towards recording empty documents as success. It looks like for the human output we only log the non-nil errors so for people using the tool directly it's not confusing. For JSON output I think it makes most sense to keep Success
status for empty docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexzielenski i updated the PR to preserve empty documents.
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
58a18e8
to
c769268
Compare
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
7521aa4
to
822dbd2
Compare
822dbd2
to
8f88d9c
Compare
@alexzielenski PTAL when you have time. |
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
thanks for adding this!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexzielenski, eddycharly 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 |
feat: add support for multiple objects in files
This PR adds support for multiple objects in files.