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

Helm: update agent configmap creation condition #2879

Merged
merged 3 commits into from
Jan 2, 2024

Conversation

QuantumEnigmaa
Copy link
Contributor

This PR updates the condition for the grafana-agent's configmap creation in the Helm chart.

Currently the agent's configmap is created whenever the agent is enabled in the values but because of this, it's impossible for a user to provide it's own custom config for the agent, even when using the agent.agent.configMap.content field.
I added another condition to allow the creation of the agent's configmap which is the following :

not .Values.agent.agent.configMap.create

This ensures that the agent's configmap in the templates is only created when one doesn't want to provide one's own custom config. Would it be the case, the user would have to set the agent.agent.configMap.create field to true and write the wanted config in the agent.agent.configMap.content field without having the current default configmap created alongside.

Also, should I bump the chart's version ?

@QuantumEnigmaa QuantumEnigmaa requested a review from a team as a code owner December 26, 2023 08:48
@CLAassistant
Copy link

CLAassistant commented Dec 26, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@kolesnikovae
Copy link
Collaborator

kolesnikovae commented Dec 27, 2023

Thank you for the contribution @QuantumEnigmaa! I agree, the way the agent is configured should be refined.

Currently the agent's configmap is created whenever the agent is enabled in the values but because of this, it's impossible for a user to provide it's own custom config for the agent, even when using the agent.agent.configMap.content field.

Users should be able to provide their own configMap by overriding the agent.configMap.name field – in this case they are fully responsible for creation/management of the config map resource. Alternatively, if a user wants to provide the config content via agent.configMap.content, to the best of my knowledge, agent.configMap.name should be empty.

However, as you mentioned, the pyroscope chart will generate a new config map resource regardless of the agent configuration.

I added another condition to allow the creation of the agent's configmap which is the following :

TBH, I'm not 100% if we should rely on this parameter as it introduces a quite implicit dependency which, strictly speaking, isn't necessary in some cases (for example, if a user brings its own configMap resource and they don't want agent/pyroscope chart to generate any extra resources).

I would consider migrating the default configuration we ship to the agent configMap via the agent.configMap.content parameter (+agent.configMap.create). In my opinion, this should remove any ambiguity.

That's being said, I have no objections to merging this PR but I would like to hear your thoughts

Also, should I bump the chart's version ?

Yes, please – you will also need to run make helm/check and make helm/docs

kolesnikovae
kolesnikovae previously approved these changes Dec 27, 2023
@QuantumEnigmaa
Copy link
Contributor Author

I would consider migrating the default configuration we ship to the agent configMap via the agent.configMap.content parameter (+agent.configMap.create). In my opinion, this should remove any ambiguity.

I agree. I'm gonna move the config there and add some comments to explain how to provide another config

Signed-off-by: QuantumEnigmaa <thibaud@giantswarm.io>
@QuantumEnigmaa
Copy link
Contributor Author

@kolesnikovae Can you check to make sure the change is suited to what you expected pls ? :)

@kolesnikovae
Copy link
Collaborator

@QuantumEnigmaa thanks! I'll take a look by the end of the year ;)

Signed-off-by: QuantumEnigmaa <thibaud@giantswarm.io>
Copy link
Collaborator

@kolesnikovae kolesnikovae left a comment

Choose a reason for hiding this comment

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

LGTM

@kolesnikovae kolesnikovae merged commit 25cf17e into grafana:main Jan 2, 2024
19 checks passed
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