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.

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

Fix handling of $NAME and $IMAGE in runlabel #9945

Merged
merged 1 commit into from
Apr 14, 2021

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Apr 5, 2021

Fixes: #9405

Add system runlabel tests.

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

@openshift-ci-robot
Copy link
Collaborator

[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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2021

run_podman build -t runlabel_image $tmpdir

run_podman container runlabel --opt1=OPT1 --opt2=OPT2 --opt3=OPT3 --name test1 --display install runlabel_image
Copy link
Member

Choose a reason for hiding this comment

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

Just a quick once-over, but this caught my attention. Could I suggest:

    opt1=opt1$(random_string 8)
    optd=opt2$(random_string 10)
    --opt1=$opt1 --opt2=$opt2

Or, at a minimum, --opt1=AAA --opt2=BBB? Seeing them as OPT1, OPT2 makes me have to triple-check the quoting in the Containerfile, make sure that OPT1 isn't appearing in the results simply as a literal.

run_podman build -t runlabel_image $tmpdir

run_podman container runlabel --opt1=OPT1 --opt2=OPT2 --opt3=OPT3 --name test1 --display install runlabel_image
is "$output" "command: bin/podman run -t -i --rm OPT1 --privileged -v /:/host --net=host --ipc=host --pid=host -e HOST=/host -e NAME=test1 -e IMAGE=localhost/runlabel_image:latest -e CONFDIR=/etc/test1 -e LOGDIR=/var/log/test1 -e DATADIR=/var/lib/test1 localhost/runlabel_image:latest OPT2 /bin/install.sh OPT3" "generating runlabel install command"
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: $PODMAN instead of bin/podman. This should cover all cases including podman-remote.

@rhatdan rhatdan force-pushed the runlabel branch 2 times, most recently from 3ac1390 to dedd729 Compare April 6, 2021 15:02
@TomSweeneyRedHat
Copy link
Member

Test failure looked like a flake, I've restarted.

Fixes: containers#9405

Add system runlabel tests.

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

rhatdan commented Apr 14, 2021

@containers/podman-maintainers PTAL

Copy link
Member

@ashley-cui ashley-cui left a comment

Choose a reason for hiding this comment

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

LGTM

@edsantiago
Copy link
Member

LGTM

@rhatdan rhatdan added the lgtm Indicates that a PR is ready to be merged. label Apr 14, 2021
@openshift-merge-robot openshift-merge-robot merged commit 4ee203d into containers:master Apr 14, 2021
@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 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

Successfully merging this pull request may close these issues.

runlabel behavior is inconsistent with documentation/atomic
6 participants