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

[aliaslanturk]

This PR is basically an alternation for legibility and minor improvement.

Short history

According to reported bug #8931, podman images fails after buildah manifest create multi:1.0.
In response to the bug, #8934 was merged with accompanying system test on test/system/010-images.bats.

Need for the PR

Proposed system test uses '[[' for output comparison.
Main goal of this PR was to utilise is() instead of '[['. However, there was a room for improvement and legibility.
Current commit checks podman images output for IDs of both old (default image) and new (new manifest test:1.0).

The good

is() function is utilised. No wildcards are used for output comparison. Even though this makes the test fragile, it provides small robustness. Also, it uses two different types of expected patterns, namely, "sha256:$iid" and "$iid_new", which would break if --format {{.ID}} output style were to change.

The bad

Test is more fragile. It may break in case of cosmetic output change. (However, could be fixed easily.)

The ugly

Commit replaces 1 line with 8 lines.

Note: I want to avoid triggering CI with tags . However, I have not learn the CI flow fully, I am reluctant to include no test hooks.

Signed-off-by: Ali Aslanturk ilaaslanturk@gmail.com

[edsantiago]

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!

[aliaslanturk]

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.

[aliaslanturk]

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
}

[edsantiago]

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?

[aliaslanturk]

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.

[edsantiago]

Did you consider podman images --sort=created ?

[aliaslanturk]

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.

[edsantiago]

@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!

[aliaslanturk]

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.

[edsantiago]

Your PR, your choice! :)

[edsantiago]

/approve

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliaslanturk, edsantiago

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 [edsantiago]

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]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliaslanturk, edsantiago

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 [edsantiago]

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

[rhatdan]

@aliaslanturk Still working on this?

[edsantiago]

@rhatdan they are unavailable during this month, but assured me of continuing interest in this PR upon their return.

[aliaslanturk]

I need to catch-up with the changes since the commit. Hence, converting to draft.

[github-actions]

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

[vrothberg]

Friendly ping to bump it in the inbox.

[aliaslanturk]

Friendly ping to bump it in the inbox.

ty for the ping. I am having problems with catching up. For this possibility I have reverted it to a draft. Sorry for blocking.

[github-actions]

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

[rhatdan]

@aliaslanturk Any progress?

[aliaslanturk]

@aliaslanturk Any progress?

Sir, there was a lot on my plate. I am trying to make space. Apologies for the delay.

[github-actions]

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

[vrothberg]

I am closing this PR due to inactivity so others can take over if time allows. Feel free to reopen once you find time working on it again.