-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add test case for description being present in search result #7141
Add test case for description being present in search result #7141
Conversation
/cherry-pick v2.0 |
Did you think about enabling the cherry-pick plugin for this repo? |
We usually use labels but it seems like a nice to have for patches that apply cleanly. @mheon WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
test/e2e/search_test.go
Outdated
for _, v := range lines { | ||
match, _ = regexp.MatchString(`^quay.io\s+quay.io/libpod/whalesay\s+Static image used for automated testing.+$`, v) | ||
if match { | ||
break | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a multi-line regex here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@vrothberg @saschagrunert We do want to do some additional CI maintenance at some point (to add flagging to PRs that change the API), so we can investigate adding the cherry-pick functionality when we're doing that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hm, tests are still failing. But it's now a different testcase. Seems that |
/lgtm |
Needs to rebase to fix the Ubuntu tests |
Test for a specific static image and match the description to avoid regression like containers#7131 Signed-off-by: Ralf Haferkamp <[email protected]>
`podman image search` returned wrong results for the image "Description" as it was mapped to the wrong field ("ID") in the search results. Basically cherry-picked into the api from commit cf5c63b. Signed-off-by: Ralf Haferkamp <[email protected]>
The HTTP API for image search was still lacking support of the NoTrunc parameter. Signed-off-by: Ralf Haferkamp <[email protected]>
LGTM |
/lgtm Will remove hold once things go green |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just got one single nit. @mheon feel free to lift the hold if it does not hurt. LGTM
search := podmanTest.Podman([]string{"search", "quay.io/libpod/whalesay"}) | ||
search.WaitWithDefaultTimeout() | ||
Expect(search.ExitCode()).To(Equal(0)) | ||
output := fmt.Sprintf("%s", search.Out.Contents()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we could avoid the Sprintf here right?
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhafer, rhatdan, saschagrunert 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 |
@rhafer Could you cleanup the sprintf problem, and then we will merge. |
Ralf is on vacation for the next 3 weeks, I can follow-up with the snprintf nit in another PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/hold cancel |
SGTM, thanks! |
Test for a specific static image and match the description to avoid
regression like #7131
Also apply the fix above to the api implementation.
Signed-off-by: Ralf Haferkamp [email protected]