Skip to content

Conversation

axel7083
Copy link
Contributor

@axel7083 axel7083 commented Jun 4, 2025

Does this PR introduce a user-facing change?

Required for #26274

podman kube play support multiple file as arguments

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note labels Jun 4, 2025
Copy link
Contributor

openshift-ci bot commented Jun 4, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: axel7083
Once this PR has been reviewed and has the lgtm label, please assign luap99 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@axel7083 axel7083 force-pushed the feature/kube-play/multi-file-support branch 4 times, most recently from 6353c5a to ad961d7 Compare June 11, 2025 09:46
Copy link

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

2 similar comments
Copy link

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

Copy link

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

@axel7083
Copy link
Contributor Author

@axel7083 axel7083 force-pushed the feature/kube-play/multi-file-support branch from e37a183 to 3a509ac Compare June 12, 2025 16:08
Copy link

A friendly reminder that this PR had no activity for 30 days.

@Honny1
Copy link
Member

Honny1 commented Oct 2, 2025

@axel7083 Hope you're doing well.

Just wanted to follow up on this PR. Are you still planning to work on it? No worries if not, I can pick it up from here if needed.

Let me know! Thanks.

@axel7083
Copy link
Contributor Author

axel7083 commented Oct 6, 2025

@axel7083 Hope you're doing well.

Just wanted to follow up on this PR. Are you still planning to work on it? No worries if not, I can pick it up from here if needed.

Let me know! Thanks.

Hey @Honny1,

Sorry I kinda forgot about this, I had a blocker with the tests, not able to fix them, if someone could take a look or takeover, would be lovely.

Thanks in advance

@Honny1
Copy link
Member

Honny1 commented Oct 6, 2025

@axel7083 I'll investigate the problem, and then we can discuss the next steps.

Copy link
Member

@Honny1 Honny1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked the test results, and the main problem seems to be with autocompletion. I think the other CI failures are just flakes.

I have suggested some fixes for the autocompletion issue and left a review. This change will also require a documentation update.

@axel7083 What do you think? Do you want to finish this PR, or should I take over?

return play(cmd, args)
}

func readerFromArgs(args []string) (*bytes.Reader, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nonblocking: This function could be optimized to reduce buffer operations and avoid reading whole files.

Comment on lines +38 to +43
files: []string{
`apiVersion: v1
kind: ConfigMap
metadata:
name: my-config`,
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nonblocking: I'm not a fan of these log strings. They visually disrupt the file's appearance. You could do something like this instead:

Suggested change
files: []string{
`apiVersion: v1
kind: ConfigMap
metadata:
name: my-config`,
},
files: []string{strings.Join([]string{
"apiVersion: v1",
"kind: ConfigMap",
"metadata:",
" name: my-config",
}, "\n"),},

}

func TestReaderFromArgs(t *testing.T) {
tests := []struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see several duplicate file definitions. This could be reduced with some constants.


It("all arguments should be read", func() {
// let's create a matrix of pod name => filename
pods := [][]string{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this matrix is needed. Instead, you can simply add or remove the .yaml extension from the podname.

}

// run the podman command
kube := podmanTest.Podman(cmd)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use podmanTest.PodmanExitCleanly().


// for each pods, let's ensure it has been created nicely
for _, test := range pods {
inspect := podmanTest.Podman([]string{"pod", "inspect", test[0]})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use podmanTest.PodmanExitCleanly() and also check the output of inspect.

RunE: down,
Args: cobra.ExactArgs(1),
Args: cobra.MinimumNArgs(1),
ValidArgsFunction: common.AutocompleteDefaultOneArg,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ValidArgsFunction: common.AutocompleteDefaultOneArg,
ValidArgsFunction: completion.AutocompleteDefault,

RunE: play,
Args: cobra.ExactArgs(1),
Args: cobra.MinimumNArgs(1),
ValidArgsFunction: common.AutocompleteDefaultOneArg,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ValidArgsFunction: common.AutocompleteDefaultOneArg,
ValidArgsFunction: completion.AutocompleteDefault,

RunE: playKube,
Args: cobra.ExactArgs(1),
Args: cobra.MinimumNArgs(1),
ValidArgsFunction: common.AutocompleteDefaultOneArg,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ValidArgsFunction: common.AutocompleteDefaultOneArg,
ValidArgsFunction: completion.AutocompleteDefault,

}

// prepare our podman command
var cmd = []string{"play", "kube"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var cmd = []string{"play", "kube"}
var cmd = []string{"kube", "play"}

@Honny1
Copy link
Member

Honny1 commented Oct 14, 2025

I am taking over this pull request, with the agreement of @axel7083. I have filed a new PR(#27292) to address it.

@Honny1 Honny1 closed this Oct 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note stale-pr

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants