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 'has' selectors #473

Merged
merged 2 commits into from
Dec 14, 2021
Merged

Fix handling of 'has' selectors #473

merged 2 commits into from
Dec 14, 2021

Conversation

antoineco
Copy link
Contributor

@antoineco antoineco commented Oct 11, 2021

Fixes #467

The first commit includes test cases which surface the issue.

The second one includes a fix, which consists in treating a missing metadata.labels as a {} map.


Test run before fix:

--- FAIL: TestMatchesSelector (0.00s)

    --- FAIL: TestMatchesSelector/single_non-labeled_object_with_not-has_selector (0.00s)
        selector_test.go:215: unexpected result: got false - want true

    --- FAIL: TestMatchesSelector/not-has_selector_matching_all_non-labeled_elements_of_list_object (0.00s)
        selector_test.go:200: unexpected result: got false - want true
        selector_test.go:209: unexpected diff (-want, +got)   (
                """
                ... // 3 identical lines
                    resourceVersion: ""
                    selfLink: ""
            -   items:
            -       - apiVersion: v1
            -         kind: Pod
            -         metadata:
            -           name: rss-site
            -       - apiVersion: v1
            -         kind: Pod
            -         metadata:
            -           name: rss-db
            +   items: []
                """
              )

@google-cla google-cla bot added the cla: yes label Oct 11, 2021
@antoineco antoineco marked this pull request as ready for review October 12, 2021 09:33
Copy link
Contributor Author

@antoineco antoineco left a comment

Choose a reason for hiding this comment

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

Notes to reviewers.

Comment on lines +41 to +43
if kind == "" {
return false, nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only case where docKind() returns an empty kind string without error is when the node is a !!null node.

I'm not sure under what circumstances such node is passed to MatchesSelector(), but I had to short-circuit here because objMatchesSelector() doesn't understand what a !!null node is.

I'm happy to remove this check on kind and add a check for !!null to objMatchesSelector() instead, in case someone feels strongly about it.

Comment on lines -142 to -146
}, {
desc: "null node",
input: "!!null",
selector: labels.Everything(),
matches: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See previous comment for the reason for this removal.

@imjasonh
Copy link
Member

Thanks for adding this! This looks fine to me, @jonjohnsonjr do you have any comments?

@jonjohnsonjr
Copy link
Collaborator

I'm not enough of a yaml engineer to have a strong opinion about this, lgtm if you think it's fine.

@jonjohnsonjr jonjohnsonjr merged commit 66a77a9 into ko-build:main Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Negated selectors don't apply to objects that have no metadata.labels
3 participants