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

Instead of adding our own label, reuse Statefulset selector #18

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

krajorama
Copy link
Contributor

@krajorama krajorama commented Sep 5, 2022

This removes the need for the mostly redundant "name" label.

Fixes: #15

Signed-off-by: György Krajcsovits [email protected]

@krajorama krajorama marked this pull request as draft September 5, 2022 11:22
@krajorama krajorama force-pushed the krajo/20220905-remove-name-label branch from 7cfab9a to e174379 Compare September 5, 2022 11:26
This removes the need for the mostly redundant "name" label.

Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
@krajorama krajorama force-pushed the krajo/20220905-remove-name-label branch from e174379 to d236af4 Compare September 5, 2022 12:10
@krajorama krajorama marked this pull request as ready for review September 5, 2022 12:11
@pstibrany
Copy link
Member

This change will use all label matchers from statefulset selector to find pods. This is possibly a breaking change. It's not clear to me why it wasn't done before (perhaps due to a scenario when deploying rollout-operator to existing cluster?).

@christopherzli
Copy link

I am wondering why this is not merged into master.

Copy link

@christopherzli christopherzli left a comment

Choose a reason for hiding this comment

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

LGTM

@pracucci
Copy link
Collaborator

This change will use all label matchers from statefulset selector to find pods. This is possibly a breaking change. It's not clear to me why it wasn't done before (perhaps due to a scenario when deploying rollout-operator to existing cluster?).

We need to better investigate this before merging this PR.

@christopherzli
Copy link

is there any help or anything I can help to confirm in order to merge this PR? In my understanding this would not be a breaking change.

@pracucci
Copy link
Collaborator

is there any help or anything I can help to confirm in order to merge this PR? In my understanding this would not be a breaking change.

We need to test it at Grafana Labs to make sure it's not a breaking change for our use cases.

@aallawala
Copy link

@pracucci, how's testing going at Grafana Labs re: this change?

@pracucci
Copy link
Collaborator

Priorities changed and we never tested this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the "name" label requirement on StatefulSet's pods
5 participants