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 multiple filters in podman ps #1345

Closed
wants to merge 2 commits into from

Conversation

mheon
Copy link
Member

@mheon mheon commented Aug 24, 2018

Docker expects multiple filters to be passed with multiple uses of the --filter flag (e.g. --filter=label=a=b --filter=label=c=d) and not a single comma-separated list of filters as we expected. Convert to the Docker format, and make some small cleanups to our handling of filters along the way.

Fixes #1341

@mheon
Copy link
Member Author

mheon commented Aug 24, 2018

Note to self: manpages and bash completions probably also need updating

Expect(result.ExitCode()).To(Equal(0))
Expect(result.OutputToStringArray()[0]).To(Equal(fullCid))
})

Copy link
Member

Choose a reason for hiding this comment

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

I think this new test might have succeeded with the old behavior. It might be good to create a second container with key1==value2 and then make sure you only get one container back.

Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

One small test tweak suggestion, otherwise LGTM. If you want to tweak the test next week in a separate PR, I'm hip.

@EmilienM
Copy link
Contributor

I tested the PR and it fixed my bug. Thanks again @mheon !

mheon added 2 commits August 27, 2018 09:12
Docker expects multiple filters to be passed with multiple uses
of the --filter flag (e.g. --filter=label=a=b --filter=label=c=d)
and not a single comma-separated list of filters as we expected.
Convert to the Docker format, and make some small cleanups to our
handling of filters along the way.

Signed-off-by: Matthew Heon <[email protected]>
@mheon
Copy link
Member Author

mheon commented Aug 27, 2018

Fixed the test and added another container to make sure the match logic for multiple filters is logical-AND, and updated the manpage. This should be ready now. CI seems to be having its usual issues though.

@mheon
Copy link
Member Author

mheon commented Aug 27, 2018

bot, retest this please

Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

LGTM assuming happy tests
Thanks for the test tweak

@mheon
Copy link
Member Author

mheon commented Aug 27, 2018

@rhatdan @umohnani8 @baude @vrothberg PTAL

@umohnani8
Copy link
Member

LGTM

@mheon
Copy link
Member Author

mheon commented Aug 27, 2018

@rh-atomic-bot r=umohnani8

@rh-atomic-bot
Copy link
Collaborator

📌 Commit 9c5f809 has been approved by umohnani8

@rh-atomic-bot
Copy link
Collaborator

⌛ Testing commit 9c5f809 with merge 9edea23...

rh-atomic-bot pushed a commit that referenced this pull request Aug 27, 2018
Signed-off-by: Matthew Heon <[email protected]>

Closes: #1345
Approved by: umohnani8
@rh-atomic-bot
Copy link
Collaborator

☀️ Test successful - status-papr
Approved by: umohnani8
Pushing 9edea23 to master...

@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 27, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants