Add --configmap to podman-remote kube play #18005

umohnani8 · 2023-03-31T19:16:09Z

Enable the --configmap flag for the remote case of podman
kube play. Users can pass in the paths to the configmap files
for kube play to use when creating the pods and containers from
a kube yaml file. The configmap file is read and the contents are
appended to the contents of the main yaml file before passed to the
remote client.

Fixes #17513

Does this PR introduce a user-facing change?

Add --configmap flag to podman-remote kube play.

umohnani8 · 2023-03-31T19:16:47Z

@vrothberg @baude was there a reason that this flag was disabled for the remote case? I tested it out locally and it works fine with podman-remote but maybe there is something I am missing.

vrothberg · 2023-04-03T07:48:08Z

@vrothberg @baude was there a reason that this flag was disabled for the remote case? I tested it out locally and it works fine with podman-remote but maybe there is something I am missing.

It probably only works because you test it on the same machine. Passing a path to the server will work in this scenario but what if the client is on another machine? What would need to happen is to tar up the file, send it over to the server and use it there.

There are other flags/features (e.g., build) that don't (yet?) work for the same reason.

Luap99 · 2023-04-03T12:52:16Z

Yes this came up before and we should not send local paths via API.
Play kube already supports configmaps as part of the same (mutli doc) yaml file, I think we should patch the remote client to read the configmaps and append them to the original yaml file which is then send via the body to the server.

For historical reference: #12243

vrothberg · 2023-04-03T12:54:18Z

I think we should patch the remote client to read the configmaps and append them to the original yaml file which is then send via the body to the server

That is a very clever idea 👍

Luap99

There are more SkipIfRemote's that should be removed in the e2e tests.

pkg/api/server/register_kube.go

test/e2e/play_kube_test.go

Luap99

LGTM, @vrothberg PTAL

vrothberg

/lgtm
/hold

openshift-ci · 2023-05-17T13:27:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: umohnani8, vrothberg

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~~ [umohnani8,vrothberg]

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

Enable the --configmap flag for the remote case of podman kube play. Users can pass in the paths to the configmap files for kube play to use when creating the pods and containers from a kube yaml file. The configmap file is read and the contents are appended to the contents of the main yaml file before passed to the remote client. Signed-off-by: Urvashi Mohnani <[email protected]>

TomSweeneyRedHat · 2023-05-17T17:02:24Z

cmd/podman/kube/play.go

@@ -161,6 +161,10 @@ func playFlags(cmd *cobra.Command) {
 	waitFlagName := "wait"
 	flags.BoolVarP(&playOptions.Wait, waitFlagName, "w", false, "Clean up all objects created when a SIGTERM is received or pods exit")

+	configmapFlagName := "configmap"
+	flags.StringSliceVar(&playOptions.ConfigMaps, configmapFlagName, []string{}, "`Pathname` of a YAML file containing a kubernetes configmap")


Nit, but it probably ought to be "Kubernetes"

Suggested change

flags.StringSliceVar(&playOptions.ConfigMaps, configmapFlagName, []string{}, "`Pathname` of a YAML file containing a kubernetes configmap")

flags.StringSliceVar(&playOptions.ConfigMaps, configmapFlagName, []string{}, "`Pathname` of a YAML file containing a Kubernetes configmap")

Will fix in a follow up PR.

TomSweeneyRedHat · 2023-05-17T17:09:04Z

test/system/700-play.bats

@@ -653,8 +653,6 @@ spec:
 }

 @test "podman kube play with configmaps" {
-    skip_if_remote "the configmap argument is supported only locally"


Love, love, love all these removals!

TomSweeneyRedHat · 2023-05-17T17:11:26Z

One nit, otherwise LGTM

umohnani8 · 2023-05-18T09:56:09Z

@containers/podman-maintainers can I please get a lgtm here

rhatdan · 2023-05-18T10:47:06Z

/lgtm
/hold

umohnani8 · 2023-05-18T14:57:14Z

/hold cancel

openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 31, 2023

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Mar 31, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2023

umohnani8 force-pushed the configmap branch from f499c47 to 21b502f Compare May 16, 2023 17:14

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2023

umohnani8 force-pushed the configmap branch 2 times, most recently from 329a66b to 1948da4 Compare May 17, 2023 12:19

Luap99 reviewed May 17, 2023

View reviewed changes

pkg/api/server/register_kube.go Outdated Show resolved Hide resolved

umohnani8 force-pushed the configmap branch from 1948da4 to 2a2f545 Compare May 17, 2023 12:50

github-actions bot removed the kind/api-change Change to remote API; merits scrutiny label May 17, 2023

Luap99 reviewed May 17, 2023

View reviewed changes

test/e2e/play_kube_test.go Show resolved Hide resolved

umohnani8 force-pushed the configmap branch from 2a2f545 to 889ed88 Compare May 17, 2023 13:09

Luap99 reviewed May 17, 2023

View reviewed changes

vrothberg approved these changes May 17, 2023

View reviewed changes

openshift-ci bot assigned vrothberg May 17, 2023

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels May 17, 2023

umohnani8 force-pushed the configmap branch from 889ed88 to a43025e Compare May 17, 2023 14:19

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 17, 2023

umohnani8 force-pushed the configmap branch from a43025e to 5ff6fc5 Compare May 17, 2023 14:24

TomSweeneyRedHat reviewed May 17, 2023

View reviewed changes

openshift-ci bot assigned rhatdan May 18, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 18, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 18, 2023

openshift-merge-robot merged commit a58ea23 into containers:main May 18, 2023

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 23, 2023

github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --configmap to podman-remote kube play #18005

Add --configmap to podman-remote kube play #18005

umohnani8 commented Mar 31, 2023 •

edited

Loading

umohnani8 commented Mar 31, 2023

vrothberg commented Apr 3, 2023

Luap99 commented Apr 3, 2023

vrothberg commented Apr 3, 2023

Luap99 left a comment

Luap99 left a comment

vrothberg left a comment

openshift-ci bot commented May 17, 2023

TomSweeneyRedHat May 17, 2023 •

edited

Loading

umohnani8 May 18, 2023

TomSweeneyRedHat May 17, 2023

TomSweeneyRedHat commented May 17, 2023

umohnani8 commented May 18, 2023

rhatdan commented May 18, 2023

umohnani8 commented May 18, 2023

	flags.StringSliceVar(&playOptions.ConfigMaps, configmapFlagName, []string{}, "`Pathname` of a YAML file containing a kubernetes configmap")
	flags.StringSliceVar(&playOptions.ConfigMaps, configmapFlagName, []string{}, "`Pathname` of a YAML file containing a Kubernetes configmap")

Add --configmap to podman-remote kube play #18005

Add --configmap to podman-remote kube play #18005

Conversation

umohnani8 commented Mar 31, 2023 • edited Loading

Does this PR introduce a user-facing change?

umohnani8 commented Mar 31, 2023

vrothberg commented Apr 3, 2023

Luap99 commented Apr 3, 2023

vrothberg commented Apr 3, 2023

Luap99 left a comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

openshift-ci bot commented May 17, 2023

TomSweeneyRedHat May 17, 2023 • edited Loading

Choose a reason for hiding this comment

umohnani8 May 18, 2023

Choose a reason for hiding this comment

TomSweeneyRedHat May 17, 2023

Choose a reason for hiding this comment

TomSweeneyRedHat commented May 17, 2023

umohnani8 commented May 18, 2023

rhatdan commented May 18, 2023

umohnani8 commented May 18, 2023

umohnani8 commented Mar 31, 2023 •

edited

Loading

TomSweeneyRedHat May 17, 2023 •

edited

Loading