Read resources from multiple files#258
Conversation
|
Hi @carmal891. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
88cd652 to
4c8175c
Compare
6605bd4 to
b92cee2
Compare
|
/ok-to-test |
b92cee2 to
d9fbcca
Compare
d9fbcca to
4206cc9
Compare
cfdb7ef to
9504ef6
Compare
|
@Stevenjin8 Kindly help to review this once again when possible. It would be much appreciated |
Stevenjin8
left a comment
There was a problem hiding this comment.
I don't think we need to deal with folders. The shell already lets you do that: path/to/my/folder/*.{json,yaml,yml}
252e284 to
fe8ed61
Compare
fe8ed61 to
3c3e85b
Compare
|
@Stevenjin8 All review suggestions are addressed |
|
/approve |
| readers = append(readers, f) | ||
|
|
||
| if i < len(allFiles)-1 { | ||
| readers = append(readers, strings.NewReader("\n---\n")) |
There was a problem hiding this comment.
Are we sure this will not break JSONs ?
There was a problem hiding this comment.
Yes this was looked into, the NewYAMLOrJSONDecoder supports multi-document streams and treats JSON as valid YAML. So when we add --- between files, the decoder simply interprets it as a document separator and cleanly decodes each object one by one. Therefore, it does not break JSON inputs.
| if statErr != nil { | ||
| return fmt.Errorf("input path does not exist: %s", path) | ||
| } | ||
| if info.IsDir() { |
There was a problem hiding this comment.
It is common for Kubernetes tools to support directory inputs by recursively searching for .yaml or .json files. Maybe we should consider the same here?
There was a problem hiding this comment.
I think as @Stevenjin8 has mentioned here, users can already achieve the same behaviour via shell expansion:
path/to/my/folder/*.{json,yaml,yml}
Since this provides equivalent functionality without adding complexity to our implementation, I think it’s reasonable to keep the current behaviour and avoid handling directories internally.
3c3e85b to
8a9e107
Compare
|
@bexxmodd I have addressed your comments. Please go through once again |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bexxmodd, carmal891, Stevenjin8 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@Stevenjin8 Can this be merged now that it has been approved by you and @bexxmodd ? |
|
@bexxmodd can you |
|
/lgtm |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Read resources from multiple files
Which issue(s) this PR fixes:
Fixes #106
Does this PR introduce a user-facing change?: