Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add grafanacloud-specific install script #291
Add grafanacloud-specific install script #291
Changes from 2 commits
d851051
b7e2964
bcd3d79
033a1ed
9b964da
c4e243f
2960322
9d826a8
e85cd41
cf4c956
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is out of scope but we really should start consolidating our GCom clients 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding
NewClientFromEnv
that assumes a configuredGCLOUD_API_KEY
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the client code is too low level for environment variable logic. The caller can be responsible for retrieving environment variables, if necessary.
(That being said, I don't really like environment variables for configuration, and I think having the flag here is enough given this is a hidden command we don't want to document)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably miss something but why not use
curl ${API_ENDPOINT}/stacks/${GCLOUD_STACK_ID}/agent.yaml
instead? The GCom client seems to be overhead unless we'll add more logic I am not aware of.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation of
cloud-config
will probably change over time when integrations are decoupled from the API implementation. I'm comfortable maintaining a tiny package on the client side that's responsible for stitching together the final config file (which is where I think the logic should live anyway - the integrations API should, ideally, just give me config for integrations and remote_write info, and the Agent should be responsible for knowing how it should be configured outside of that scope).It may turn out that we even remove the package later on, but I suspect it's also where the C&C client-side code will be written.