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

Utilising is() function instead of [[. Avoiding wildcard for comparison #9916

Closed
wants to merge 1 commit into from
Closed

Utilising is() function instead of [[. Avoiding wildcard for comparison #9916

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions test/system/010-images.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,9 +236,16 @@ Labels.created_at | 20[0-9-]\\\+T[0-9:]\\\+Z
iid=$output

run_podman manifest create test:1.0
run_podman images --format '{{.ID}}' --no-trunc
[[ "$output" == *"sha256:$iid"* ]]

# Check sha256 ID of test:1.0 and $IMAGE
run_podman images --format '{{.ID}}' --no-trunc $IMAGE
is "$output" \
"sha256:$iid" \
"podman images - bare manifest list (old image)"
run_podman images --format '{{.ID}}' --no-trunc test:1.0
iid_new=$output
is "$output" \
"$iid_new" \
Comment on lines +245 to +247
Copy link
Member

Choose a reason for hiding this comment

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

This is a NOP: if you assign iid_new=$output in line 245, it is pretty certain that they will be identical in line 246 :-). What did you intend to check here?

PS Glad to see you back! Thank you for this!

Copy link
Author

@aliaslanturk aliaslanturk Apr 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

OH No! I was aiming to test that both old and new images' IDs are available in the output. I think, for the root bug #8931 just one is enough. I should have used lines to parse multiple sha outputs. This is currently a simple output check, which is done in previous is(). Or the second one can/may be avoided.

Copy link
Author

Choose a reason for hiding this comment

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

Following correction might be better. Yet, line deletion, with sed, could be handled in a better and configurable way.

@test "podman images - bare manifest list" {
    # Create an empty manifest list and list images.
    run_podman inspect --format '{{.ID}}' $IMAGE
    iid=$output
    run_podman manifest create test:1.0
    iid_new="sha256:$output"
    # Check sha256 ID of test:1.0 and $IMAGE
    run_podman images --format '{{.ID}}' --no-trunc $IMAGE
    is  "$output|sed '1d'" \ 
        "sha256:$iid" \
        "podman images - bare manifest list (old image)"
    run_podman images --format '{{.ID}}' --no-trunc test:1.0
    is  "$output" \
        "$iid_new" \
        "podman images - bare manifest list (new image)"
    run_podman rmi test:1.0
}

Copy link
Member

Choose a reason for hiding this comment

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

is "$output|sed '1d'" \

This does not do what you think it does. I think you meant:

    is "$(sed -e 1d <<<"$output")
    is "$(tail -n1 <<<"$output")    ! better: 'tail' is more intuitive in this context
    is "${lines[1]}"                ! best: see $lines in bats(7)

However, this still makes me uncomfortable because it relies on localhost/test:1.0 sorting before $IMAGE. This is true today, but might not be tomorrow ($IMAGE might change, or podman's sorting behavior). I would prefer to see the manifest created as zzz.zzz/test:1.0, with run_podman images --sort repository then comparing ${lines[0]} and [1] sequentially. WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

Following would work, I think. As long as $IMAGE is defined and test:1.0 is kept unique, it will work. Using --format '{{.ID}}' --no-trunc IMAGE_NAME will avoid multi-line output and the need for repository information.

What it contributes differently is that iids are collected and checked at different times.

@test "podman images - bare manifest list" {
    # Create an empty manifest list and list images.
    run_podman inspect --format '{{.ID}}' $IMAGE
    iid=$output
    run_podman manifest create test:1.0
    iid_new="sha256:$output"

    # Check sha256 ID of test:1.0 and $IMAGE
    run_podman images --format '{{.ID}}' --no-trunc $IMAGE
    is  "$output" \
        "sha256:$iid" \
        "podman images - bare manifest list (old image)"
    run_podman images --format '{{.ID}}' --no-trunc test:1.0
    is  "$output" \
        "$iid_new" \
        "podman images - bare manifest list (new image)"
    run_podman rmi test:1.0
}

If we were to check pure podman images command (which would be an elegant way to test), I agree, what you have suggested would work. However, we started to move away from a regression test. Following might help:

@test "podman images - bare manifest list" {
    # Create an empty manifest list and list images.
    run_podman inspect --format '{{.ID}}' $IMAGE
    iid=$output
    run_podman manifest create test:1.0
    iid_new="sha256:$output"

    #Tag the new manifest to add distinguishable repository
    run_podman tag $iid_new zzz.zzz/test:1.0
    run_podman rmi localhost/test:1.0

    # Check sha256 ID of test:1.0 and $IMAGE
    run_podman images --no-trunc --sort repository --format '{{.ID}}'
    is "${lines[0]}"\
       "sha256:$iid"\
       "podman images - bare manifest list (old image)"
    is "${lines[1]}"\
       "$iid_new"\
       "podman images - bare manifest list (new image)"

    run_podman rmi test:1.0
}

This cross checks for sha256 text. Yet, I could not find a way to remove localhost as the repository other than tag & rmi.

Copy link
Member

Choose a reason for hiding this comment

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

Did you consider podman images --sort=created ?

Copy link
Author

Choose a reason for hiding this comment

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

It would be useful. However, similarly, tests become susceptible to changes. Question remains, how much we want to diverge from original structure ? I'd definitely prefer that way via which a more concrete testing would be possible.

Copy link
Member

Choose a reason for hiding this comment

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

@aliaslanturk at this point I'm too confused about what we're talking about... if you'd like to rebase and resubmit a new set of diffs, I will gladly take a look at that!

Copy link
Author

Choose a reason for hiding this comment

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

I am just overthinking it. Sorry :)
Both --sort=created and --sort=repository would work, I believe. Check with repository seems to be more robust. I could re-submit with any one of the two.

Copy link
Member

Choose a reason for hiding this comment

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

Your PR, your choice! :)

"podman images - bare manifest list (new image)"
run_podman rmi test:1.0
}
# vim: filetype=sh