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

Switch Topology defaultLabelSelector to lib-common GetLabelSelector #286

Conversation

fmount
Copy link
Contributor

@fmount fmount commented Feb 27, 2025

No description provided.

@fmount fmount requested review from olliewalsh and stuggi February 27, 2025 08:28
@fmount fmount force-pushed the topology_labelselector branch from a73f9c0 to 5951bae Compare February 27, 2025 08:33
Copy link
Collaborator

@gibizer gibizer left a comment

Choose a reason for hiding this comment

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

Is it a bug fix or just a refactor? If the former then it would be nice to have some test coverage for it.

@fmount
Copy link
Contributor Author

fmount commented Feb 27, 2025

Is it a bug fix or just a refactor? If the former then it would be nice to have some test coverage for it.

Not a bug, we just realized during a meeting that it would be better, when we pass the defaultLabelSelector, to match the whole set of labels associated to a deployment or a statefulset, to avoid any scheduling risk for other components matching the same label (because it was a single label). I think this simply switches to the new method, but the behavior is unchanged.

@mrkisaolamb
Copy link
Contributor

I think we have test coverage here https://github.com/openstack-k8s-operators/placement-operator/blob/main/tests/functional/placementapi_controller_test.go#L499

@fmount
Copy link
Contributor Author

fmount commented Feb 27, 2025

@gibizer @mrkisaolamb are you ok to land this patch? I think I need this update due to the hard timeline, wondering if we can approve it

@mrkisaolamb mrkisaolamb requested a review from gibizer February 27, 2025 12:55
@mrkisaolamb
Copy link
Contributor

I'm ok with this changes because IMO this is tested but lets wait for @gibizer rsponse

Copy link
Contributor

@bogdando bogdando left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

openshift-ci bot commented Feb 27, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bogdando, fmount

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

@openshift-merge-bot openshift-merge-bot bot merged commit 0d4e5e1 into openstack-k8s-operators:main Feb 27, 2025
6 checks passed
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