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 CRS e2e helper with multiple bindings #10191

Merged

Conversation

jimmidyson
Copy link
Member

@jimmidyson jimmidyson commented Feb 22, 2024

Check all bindings to see if the resource is applied. If multiple ClusterResourceSets match a cluster, the ClusterResourceSetBinding will have multiple bindings and so only the ResourceSetBinding related to the specific ClusterResourceSet must be checked.

I discovered this when using the e2e test framework in an external project that applied multiple ClusterResourceSets to a cluster as part of the e2e tests.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label labels Feb 22, 2024
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 22, 2024
@jimmidyson
Copy link
Member Author

/area e2e-testing

@k8s-ci-robot k8s-ci-robot added area/e2e-testing Issues or PRs related to e2e testing and removed do-not-merge/needs-area PR is missing an area label labels Feb 22, 2024
@jimmidyson jimmidyson changed the title test(e2e): Fix CRS helper with multiple bindings 🌱 Fix CRS e2e helper with multiple bindings Feb 22, 2024
@jimmidyson jimmidyson force-pushed the jimmi/e2e-test-crs-binding-check branch 3 times, most recently from bbfb1ef to 45b38cc Compare February 22, 2024 11:09
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 22, 2024
@jimmidyson jimmidyson force-pushed the jimmi/e2e-test-crs-binding-check branch from 45b38cc to f21b253 Compare February 22, 2024 12:59
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 22, 2024
Check the relevant ResourceSetBinding to see if the resource is applied. If multiple ClusterResourceSets match a cluster, the
ClusterResourceSetBinding will have multiple bindings and so only the
relevant ResourceSetBinding should be checked.
@jimmidyson jimmidyson force-pushed the jimmi/e2e-test-crs-binding-check branch from f21b253 to 78e9c3f Compare February 22, 2024 17:57
Copy link
Member

@chrischdi chrischdi left a comment

Choose a reason for hiding this comment

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

Last round of nits: we don't need the bool, nil checking should be enough.

test/framework/clusterresourceset_helpers.go Outdated Show resolved Hide resolved
test/framework/clusterresourceset_helpers.go Outdated Show resolved Hide resolved
@jimmidyson
Copy link
Member Author

Thanks for your reviews @chrischdi! I've applied your suggestions.

Copy link
Member

@chrischdi chrischdi left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 23, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 21eac5de9689f3b912402156c23bfc82fbe4741d

Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

Could we add a test?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2024
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 27, 2024
@jimmidyson
Copy link
Member Author

@vincepri

Could we add a test?

I've added a test for the utility function but not for the full helper as that will be a lot more involved. I don't see tests for any of the other helpers so hope that's ok. Let me know what you think. 🙏

@jimmidyson
Copy link
Member Author

@vincepri @chrischdi Would you be able to take another look please? 🙏

@chrischdi chrischdi added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 14, 2024
Copy link
Member

@chrischdi chrischdi left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 14, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: ffe0051d457154c63ed9c68d543479d93275c821

@fabriziopandini
Copy link
Member

Thanks for fixing this!
/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 19, 2024
@k8s-ci-robot k8s-ci-robot merged commit 6daf4ac into kubernetes-sigs:main Mar 19, 2024
22 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.7 milestone Mar 19, 2024
Dhairya-Arora01 pushed a commit to Dhairya-Arora01/cluster-api that referenced this pull request May 25, 2024
* test(e2e): Fix CRS helper with multiple bindings

Check the relevant ResourceSetBinding to see if the resource is applied. If multiple ClusterResourceSets match a cluster, the
ClusterResourceSetBinding will have multiple bindings and so only the
relevant ResourceSetBinding should be checked.

* fixup! refactor: Apply review suggestions

* fixup! test: Add test for CRS e2e helper
Dhairya-Arora01 pushed a commit to Dhairya-Arora01/cluster-api that referenced this pull request May 25, 2024
* test(e2e): Fix CRS helper with multiple bindings

Check the relevant ResourceSetBinding to see if the resource is applied. If multiple ClusterResourceSets match a cluster, the
ClusterResourceSetBinding will have multiple bindings and so only the
relevant ResourceSetBinding should be checked.

* fixup! refactor: Apply review suggestions

* fixup! test: Add test for CRS e2e helper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/e2e-testing Issues or PRs related to e2e testing cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants