-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add multi-file support to podman kube play/down
#27292
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
Add multi-file support to podman kube play/down
#27292
Conversation
80a686e
to
45988c5
Compare
cc @axel7083 |
The CI is failing due to an unrelated test: |
cmd/podman/kube/play.go
Outdated
} | ||
return response.Body, nil | ||
default: | ||
return os.Open(fileName) |
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.
It just struck me that we're using http.Get()
and I don't think we've set it to use TLS? Should we? Also, can this Get ever hang and never timeout?
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.
It should automatically use TLS. I'm not sure about the request timeout, though. I just reused the same function as before, which we're already using in multiple places. Is this considered bad practice in Go?
A couple of questions for consideration. |
cmd/podman/kube/play.go
Outdated
reader = f | ||
|
||
if i < len(args)-1 { | ||
combined.WriteString("\n---\n") |
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.
Please add a comment on top that this is file seperator ? Maybe also have \n---\n
in a const string
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 have added a comment and moved it into a const string.
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.
Just one comment, PR looks good overall
45988c5
to
6e57e92
Compare
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
6e57e92
to
62c5e9b
Compare
/packit retest-failed |
cmd/podman/kube/play.go
Outdated
return bytes.NewReader(combined.Bytes()), nil | ||
} | ||
|
||
func readerFromArg(fileName string) (io.ReadCloser, 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.
fileName is confusing here, it can do files or URLs
What happens if the user gives us a bunch of files, but one of them isn't valid YAML. Do we fail with a parse error? |
/approve |
Yes, we fail with parse error. |
Signed-off-by: axel7083 <[email protected]> fix: update kube play command cobra use Signed-off-by: axel7083 <[email protected]> Add multi-file support to podman kube play/down - Support multiple YAML files and URLs in single command - Combine files with YAML document separators (---) - Refactor for better testability with dependency injection - Update documentation with examples for multiple inputs - Improve memory efficiency with streaming I/O operations Fixes: containers#26274 Fixes: https://issues.redhat.com/browse/RUN-3586 Signed-off-by: Jan Rodák <[email protected]>
62c5e9b
to
9bda788
Compare
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
var configMapYAML = strings.Join([]string{ | ||
"apiVersion: v1", | ||
"kind: ConfigMap", | ||
"metadata:", | ||
" name: my-config", | ||
"data:", | ||
" key: value", | ||
}, "\n") | ||
|
||
var podYAML = strings.Join([]string{ | ||
"apiVersion: v1", | ||
"kind: Pod", | ||
"metadata:", | ||
" name: my-pod", | ||
}, "\n") | ||
|
||
var serviceYAML = strings.Join([]string{ | ||
"apiVersion: v1", | ||
"kind: Service", | ||
"metadata:", | ||
" name: my-service", | ||
}, "\n") | ||
|
||
var secretYAML = strings.Join([]string{ | ||
"apiVersion: v1", | ||
"kind: Secret", | ||
"metadata:", | ||
" name: my-secret", | ||
}, "\n") | ||
|
||
var namespaceYAML = strings.Join([]string{ | ||
"apiVersion: v1", | ||
"kind: Namespace", | ||
"metadata:", | ||
" name: my-namespace", | ||
}, "\n") |
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.
nit: this is quite ugly, go supports multi line strings just fine with the backtick escapes which would make this more readable, but not worth to push again just for this
want := strings.TrimSpace(tt.expected) | ||
|
||
if got != want { | ||
t.Errorf("unexpected output:\n--- got ---\n%s\n--- want ---\n%s", got, want) |
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.
another nit: we already depend on assert so might as well use the nicer test hallper such as assert.Equal(), etc...
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Honny1, Luap99, mheon 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 |
@Luap99 Should I fix nits? |
No its fine like this |
This PR enables
podman kube play
andpodman kube down
to accept multiple YAML files and URLs in a single command.Fixes: #26274
Fixes: https://issues.redhat.com/browse/RUN-3586
Does this PR introduce a user-facing change?