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

move prune filter parsing to common #1088

Merged
merged 1 commit into from
Jul 16, 2022
Merged

Conversation

cdoern
Copy link
Contributor

@cdoern cdoern commented Jul 11, 2022

there was some eerily similar code in c/common and in podman for
creating filter functions for various types. Move some of it here
and add support for the label!= filter in libimage and libnetwork that basically creates the inverse
function of label=

after this merges, will file a PR with the fix for containers within podman as well

there is already a label!= test in libimage/filters_test. Libimage somehow lets this syntax slide
even though it does not actually imact anything in podman

see containers/podman#14182

Signed-off-by: Charlie Doern [email protected]

pkg/filters/filters.go Outdated Show resolved Hide resolved
filter = filterLabel(ctx, value)

filter = filterLabel(ctx, value, false)
case "label!":
Copy link
Member

Choose a reason for hiding this comment

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

Can you extend the tests in fitlers_test.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vrothberg, there are already tests in libimage/filters_test.go using label! I have no idea how they were passing though....

Copy link
Member

Choose a reason for hiding this comment

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

Looking at line 101 I don't think this is the right thing to do here. Can you investigate?

Copy link
Contributor Author

@cdoern cdoern Jul 15, 2022

Choose a reason for hiding this comment

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

@vrothberg oh i see, I think the issue is that when split, negate is correctly set to true, but we still go through the switch where the default is to error out. I am going to remove that whole negate block since it does not really do anything.

edit: actually I think this works. The issue must just be with libnetwork. Shouldn't these all share helper functions since the code is virtually the same?

pkg/filters/filters.go Outdated Show resolved Hide resolved
there was some eerily similar code in c/common and in podman for
creating filter functions for various types. Move some of it here
and add support for the label!= filter in libnetwork only. Libimage already supports it.

after this merges, will file a PR with the fix for containers within podman as well

see containers/podman#14182

Signed-off-by: Charlie Doern <[email protected]>
@cdoern
Copy link
Contributor Author

cdoern commented Jul 15, 2022

ok @vrothberg @rhatdan @Luap99 I narrowed it down to just a libnetwork issue. which makes sense since, adding some debugs in podman's system prune shows that libimage parses the label! correctly and it fails on libnetwork.

libnetwork and podman's ContainerPrune generation of filter funcs are very very similar. but there is nothing I can really do about it since one returns a func of type net and the other of type container.

@cdoern cdoern marked this pull request as ready for review July 15, 2022 13:58
@rhatdan
Copy link
Member

rhatdan commented Jul 15, 2022

LGTM

@rhatdan
Copy link
Member

rhatdan commented Jul 15, 2022

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 15, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, rhatdan

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 16, 2022
@openshift-ci openshift-ci bot merged commit fc74878 into containers:main Jul 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants