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: Add support for fetching config from env #1333

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

abhisek
Copy link

@abhisek abhisek commented Jan 31, 2025

I am following the official documentation to setup Grafana with datasource-syncer for Managed Prometheus:
https://cloud.google.com/stackdriver/docs/managed-prometheus/query#use-kubernetes

Our standard approach for providing config & secrets are through environment. Adding environment as a fallback source for config when not provided through command line

@bernot-dev bernot-dev self-requested a review January 31, 2025 21:23
@bernot-dev
Copy link
Collaborator

This seems like a reasonable feature to support.

I'm not sure we would just want to support providing just a subset of the flags though. Any particular rationale for those three?

Also, the commit message lister will require a lowercase "A".

Thanks for the PR!

@abhisek abhisek force-pushed the feat/add-env-config-source branch from 7df32e2 to ac8a547 Compare February 1, 2025 12:47
@abhisek
Copy link
Author

abhisek commented Feb 1, 2025

This seems like a reasonable feature to support.

Thanks. This will help us setup the datasource-syncer within our k8s while maintaining single source of truth for config (i.e. our env providers).

I'm not sure we would just want to support providing just a subset of the flags though. Any particular rationale for those three?

Specifically for our use, the CA/Key file flags are not required from environment because we can make them available as volume mounts with any file path. A k8s secret is mounted as a file and the file name is a local concern (i.e. within the job yaml) hence the actual path does not matter and can hardcoded as a command line argument.

However I have added env fallback for ca_file, cert_file, key_file for consistency in user experience. I have consciously decided not to add a fallback for gcmEndpointOverride because I feel the defaults are appropriate. In rare cases where it needs to be overridden I think we should force its explicit usage through command line .

Also, the commit message lister will require a lowercase "A".

Fixed.

Thanks for the PR!

Copy link
Collaborator

@bernot-dev bernot-dev left a comment

Choose a reason for hiding this comment

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

Just a couple small issues around naming to resolve. Thank you!

variables:

- `DATASOURCE_UIDS`
- `GRAFANA_SERVICE_ACCOUNT_TOKEN`
Copy link
Collaborator

Choose a reason for hiding this comment

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

The corresponding flag is named grafana-api-token. The env variable name should be GRAFANA_API_TOKEN, for consistency.

- `PROJECT_ID`
- `TLS_CERT_FILE`
- `TLS_KEY_FILE`
- `TLS_CA_FILE`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Likewise, I think naming the TLS environment variables consistently with the flags would be preferred.

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.

2 participants