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

feat(authentik): Add custom pod labels #200

Closed
wants to merge 1 commit into from

Conversation

gabe565
Copy link

@gabe565 gabe565 commented Sep 11, 2023

This PR adds podLabels to values.yaml, which lets a user define labels to add to the server and worker pods.

@gabe565 gabe565 requested a review from a team as a code owner September 11, 2023 19:43
@rissson
Copy link
Member

rissson commented Sep 11, 2023

I'm a bit wary to add more stuff that doesn't differentiate between the server and worker deployments.

I'd rather have the top-level podLabels only apply to server pods, and then have worker.podLabels for the worker, like it's done with priorityClassName and worker.priorityClassName. On the other hand, that also makes it confusing as that's not what happens with podAnnotations.

Thoughts @BeryJu?

@BeryJu
Copy link
Member

BeryJu commented Sep 11, 2023

I'm a bit wary to add more stuff that doesn't differentiate between the server and worker deployments.

I'd rather have the top-level podLabels only apply to server pods, and then have worker.podLabels for the worker, like it's done with priorityClassName and worker.priorityClassName. On the other hand, that also makes it confusing as that's not what happens with podAnnotations.

I agree, I think we should be separating this going forward (ideally I would've started having three options common.podLabels, server.podLabels and worker.podLabels, but that would be quite a big change and quite breaking)
We can change podAnnotations in a future release to only apply to either one

@gabe565
Copy link
Author

gabe565 commented Sep 11, 2023

Ha amazing. I also agree, I originally set this up with worker.podLabels and server.podLabels, but decided to make it match podAnnotations for clarity. I'll go ahead and split it up again, while also adding a common key.

I may open another PR afterwards to add server.podAnnotations and worker.podAnnotations, with the existing podAnnotations applying to both so it's not a breaking change.

@rissson
Copy link
Member

rissson commented Jan 5, 2024

Superseded by #230

@rissson rissson closed this Jan 5, 2024
@rissson rissson mentioned this pull request Jan 9, 2024
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.

3 participants