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(helm-chart): Add additionalLabels for deployments #1400

Merged
merged 1 commit into from
Sep 15, 2024
Merged

feat(helm-chart): Add additionalLabels for deployments #1400

merged 1 commit into from
Sep 15, 2024

Conversation

adberger
Copy link
Contributor

This PR adds the ability to add additional Labels for the deployments.

@akselleirv
Copy link
Collaborator

Hello @adberger,

Thank you for the contribution! Can you please clarify if you intended to add the additional labels to the controller or to the runner? Currently, you have added the configuration under the runner block in the values.yaml file (Values.runner.additionalDeploymentLabels), but in the deployment template, the labels are added to the controller, not the runner.

@akselleirv akselleirv self-requested a review July 26, 2024 10:45
@adberger
Copy link
Contributor Author

Hello @adberger,

Thank you for the contribution! Can you please clarify if you intended to add the additional labels to the controller or to the runner? Currently, you have added the configuration under the runner block in the values.yaml file (Values.runner.additionalDeploymentLabels), but in the deployment template, the labels are added to the controller, not the runner.

Huh? I intend to add them to both components.
It's also like this in the commits of this PR or am I missing something?

@ilithanos
Copy link
Collaborator

@adberger I think you are misunderstanding how the controller works with the helm chart based on the changes here and what you are trying to achieve.

as @akselleirv mentioned above the labels you are currently setting in the helm chart are only for the controller and branch planner, and not for the runners themselves. So the location within values.yaml doesn't make that much sense.

Labels on runners are managed through the runnerPodTemplates and can be set on each terraform CRD resource directly. Runners are not spun up by the helm chart, but by the controller.

What exactly is it you are trying to achieve with this change? as there might already be a way of doing it within the current helm chart and controller framework.

Signed-off-by: Adrian Berger <adrian.berger@bedag.ch>
@adberger
Copy link
Contributor Author

@adberger I think you are misunderstanding how the controller works with the helm chart based on the changes here and what you are trying to achieve.

as @akselleirv mentioned above the labels you are currently setting in the helm chart are only for the controller and branch planner, and not for the runners themselves. So the location within values.yaml doesn't make that much sense.

Labels on runners are managed through the runnerPodTemplates and can be set on each terraform CRD resource directly. Runners are not spun up by the helm chart, but by the controller.

What exactly is it you are trying to achieve with this change? as there might already be a way of doing it within the current helm chart and controller framework.

Thanks for the clarification, know I think I get it.
I've updated the PR.

Our use case is to monitor the deployments with the kube-prometheus-stack where we want to opt-in monitoring of deployments by label.

@adberger
Copy link
Contributor Author

@ilithanos Any update?

@akselleirv
Copy link
Collaborator

Hi @adberger, if you are trying to scrape metrics of the tofu-controller then you can enable that in the values.yaml file. Then it will create a ServiceMonitor resource which kube-prometheus-stack will pickup. I'm not too familiar with the branch-planner part of this project, but it seems like that does not expose any prometheus metrics to scrape.

I'll close this PR if your able to scrape the tofu-controller with the existing config.

@adberger
Copy link
Contributor Author

enable that in the values.yaml file

I'm aware of the ServiceMonitor to scrape the metrics of the tofu-controller.

Our use case is a different one:
We want to monitor deployments in the cluster (kube-state-metrics).
Since we don't want to monitor all deployments we've made a query filter to filter based on labels of the resource (deployment).

What is the problem of adding a value for additional labels on the deployment in general?

@akselleirv
Copy link
Collaborator

enable that in the values.yaml file

I'm aware of the ServiceMonitor to scrape the metrics of the tofu-controller.

Our use case is a different one: We want to monitor deployments in the cluster (kube-state-metrics). Since we don't want to monitor all deployments we've made a query filter to filter based on labels of the resource (deployment).

What is the problem of adding a value for additional labels on the deployment in general?

I see. In your earlier comments you said kube-prometheus-stack and not kube-state-metrics which was the reason for my comment.

@akselleirv akselleirv merged commit 867f896 into flux-iac:main Sep 15, 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