Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add --context-dir option to podman play kube #12913

Merged
merged 1 commit into from
Mar 9, 2022
Merged

Add --context-dir option to podman play kube #12913

merged 1 commit into from
Mar 9, 2022

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Jan 18, 2022 •
edited
Loading

This option was requested so that users could specify alternate
locations to find context directories for each image build. It
requites the --build option to be set.

Partial Fix: #12485

Signed-off-by: Daniel J Walsh [email protected]

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 18, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2022
@rhatdan
Copy link
Member Author

rhatdan commented Jan 18, 2022

@edsantiago PTAL, BTW is there any easy way to change working directory to $PODMAN_TMPDIR, I would like to add a duplicate test where podman play kube --build works without the --context-dir.

@edsantiago
Copy link
Member

I think, but am not 100% sure, that each subtest runs in a subshell so you can feel free to cd to your heart's content. This is already done in the tests:

podman/test/system/070-build.bats

Line 318 in 094665a

cd $PODMAN_TMPDIR

edsantiago
edsantiago reviewed Jan 18, 2022
View reviewed changes
test/system/700-play.bats Outdated
Comment on lines 173 to 175
if is_remote; then
skip "--build is not supported in context remote"
fi
Copy link
Member

Choose a reason for hiding this comment

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

Shorter & sweeter: skip_if_remote "--build is not supported in remote context"

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to see if this passes, then I will make that change, and attempt to cd PODMAN_TMPDIR again.

@rhatdan
Copy link
Member Author

rhatdan commented Jan 18, 2022

When I cd PODMAN_TMPDIR, run_podman blows up because it no longer finds podman?

@edsantiago
Copy link
Member

When I cd PODMAN_TMPDIR, run_podman blows up because it no longer finds podman?

I'm assuming you're not using hack/bats. Try using hack/bats.

@rhatdan
Copy link
Member Author

rhatdan commented Jan 18, 2022

My own stupidity.

PODMAN=./bin/podman hack/bats -f "--build" test/system/700-play.bats
versus
PODMAN=$PWD/bin/podman hack/bats -f "--build" test/system/700-play.bats

@rhatdan
Copy link
Member Author

rhatdan commented Jan 18, 2022

hack/bats forces root for some reason, BTW but regular bats when I send in the complete path.

@edsantiago
Copy link
Member

Nonono, hack/bats is a wrapper, it tries to do everything for you so you don't need to sweat the details.

$ hack/bats play:build     <--- 'play' is the test filename, 'build' is the filter
...
$ hack/bats --help

It runs both root and rootless, because there are always differences between the two. It sets $PODMAN, and does so with an absolute path because (ahem) I may have once or twice or a million times done that same thing where I forget about cd. And it sets a few more magic vars like for the network bin helpers.

mheon
mheon reviewed Jan 19, 2022
View reviewed changes
cmd/podman/play/kube.go

if !registry.IsRemote() {
Copy link
Member

Choose a reason for hiding this comment

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

You're allowing SignaturePolicy for podman-remote now - is that expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

No this just joined the out block of if !registry.IsRemote()

nalind
nalind reviewed Jan 19, 2022
View reviewed changes
docs/source/markdown/podman-play-kube.1.md Outdated

#### **--cert-dir**=*path*

Use certificates at *path* (\*.crt, \*.cert, \*.key) to connect to the registry. (Default: /etc/containers/certs.d)
Please refer to containers-certs.d(5) for details. (This option is not available with the remote Podman client)

#### **--context-dir**=*path*
Copy link
Member

Choose a reason for hiding this comment

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

Are these supposed to be listed alphabetically?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Member

Choose a reason for hiding this comment

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

This should be after --configmap, then.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved

@nalind
Copy link
Member

nalind commented Jan 19, 2022

This doesn't look like what #12485 (comment) is describing. Is it sufficient to close the issue?

@rhatdan rhatdan added the 4.1 label Jan 27, 2022
@edsantiago edsantiago mentioned this pull request Feb 7, 2022
Closed
@rhatdan rhatdan force-pushed the kube branch 2 times, most recently from 41efed0 to f52dd4f Compare February 22, 2022 21:34
@rhatdan
Add --context-dir option to podman play kube
675d775 
This option was requested so that users could specify alternate
locations to find context directories for each image build. It
requites the --build option to be set.

Partion Fix: containers#12485

Signed-off-by: Daniel J Walsh <[email protected]>
@umohnani8
Copy link
Member

LGTM

@baude
Copy link
Member

baude commented Mar 9, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 9, 2022
@openshift-merge-robot openshift-merge-robot merged commit acfcecf into containers:main Mar 9, 2022
@github-actions 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 Sep 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@mheon mheon mheon left review comments

@nalind nalind nalind left review comments

@edsantiago edsantiago edsantiago left review comments

Assignees

@baude baude

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

When using podman kube play, allow to specify Dockerfile/build folder location
8 participants
@rhatdan @edsantiago @nalind @umohnani8 @baude @mheon @TomSweeneyRedHat @openshift-merge-robot