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

test/system: podman run with log-opt option #12221

Closed
wants to merge 1 commit into from

Conversation

chuanchang
Copy link
Contributor

This test case is used for covering rhbz#1763007.

Signed-off-by: Alex Jia [email protected]

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

This test case is used for covering rhbz#1763007.

Signed-off-by: Alex Jia <[email protected]>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 8, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chuanchang
To complete the pull request process, please assign giuseppe after the PR has been reviewed.
You can assign the PR to them by writing /assign @giuseppe in a comment when ready.

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

@rhatdan
Copy link
Member

rhatdan commented Nov 8, 2021

LGTM

Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

Some suggestions. And please don't forget to update the commit message.

@test "podman run with log-opt option" {
run_podman run -d --rm --log-opt max-size=1m $IMAGE sleep 5
cid=$output
run_podman inspect --format "{{ .Config.CreateCommand }}" $cid
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to check {{.HostConfig.LogConfig.Size}}' instead? CreateCommand` is just basically a parrot check; it doesn't actually confirm that the option was parsed properly.

@@ -736,4 +736,14 @@ EOF
is "$output" "$random_1" "output matches STDIN"
}

# rhbz#1763007 : the --log-opt for podman run does not work as expected
@test "podman run with log-opt option" {
run_podman run -d --rm --log-opt max-size=1m $IMAGE sleep 5
Copy link
Member

Choose a reason for hiding this comment

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

It can be useful to test with variable settings. Suggestion:

    # Pseudorandom size of the form N.NNN. The '| 1' handles '0.NNN' or 'N.NN0',
    # which podman displays as 'NNN kB' or 'N.NN MB' respectively.
    size=$(printf "%d.%03d" $(($RANDOM % 10 | 1)) $(($RANDOM % 100 | 1)))
    run_podman run -d --rm --log-opt max-size=${size}m $IMAGE sleep 5
    cid=$output
    run_podman inspect --format "{{ .HostConfig.LogConfig.Size }}" $cid
    is "$output" "${size}MB"

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

This test doesn't verify anything. You are checking the stored create command with is just os.Args. There is absolutely no guarantee that this value is used. I think a proper test should set the limit to something very low and write more bytes than the max size and then check that the file does not exceed the limit.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 21, 2021

@chuanchang: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@umohnani8
Copy link
Member

@chuanchang please rebase

@github-actions
Copy link

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

rhatdan added a commit to rhatdan/podman that referenced this pull request Dec 31, 2021
This test case is used for covering rhbz#1763007.
Replaces: containers#12221

Signed-off-by: Alex Jia <[email protected]>
Signed-off-by: Daniel J Walsh <[email protected]>
@rhatdan rhatdan closed this Dec 31, 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 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. stale-pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants