-
Notifications
You must be signed in to change notification settings - Fork 50
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 env vars to the grafana connection of post_start script. #105
Conversation
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.
Thanks for the PR!
Can you please rebase the PR to avoid the merge commit? And give the commit a proper commit message?
@@ -1,12 +1,20 @@ | |||
#!/bin/sh | |||
|
|||
# Grafana connection configuration |
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.
nit: comments should always be full sentences, including punctuation.
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.
Changed to "# Set your Grafana instance's connection configuration.".
echo "Setting Grafana default dashboard..." | ||
DASH_UID="sJUFc-NWk" | ||
DASH_ID=0 | ||
for i in 1 2 3 4 5; do | ||
curl -H 'Content-Type: application/json' -X GET http://admin:admin@grafana:3000/api/dashboards/uid/$DASH_UID && RESP=$(curl -H 'Content-Type: application/json' -X GET http://admin:admin@grafana:3000/api/dashboards/uid/$DASH_UID) && DASH_ID=$( echo "$RESP" | jq '.dashboard.id' ) && break || sleep 15; | ||
curl -H 'Content-Type: application/json' -X GET http://${GRAFANA_URI}/api/dashboards/uid/$DASH_UID && RESP=$(curl -H 'Content-Type: application/json' -X GET http://${GRAFANA_URI}/api/dashboards/uid/$DASH_UID) && DASH_ID=$( echo "$RESP" | jq '.dashboard.id' ) && break || sleep 15; |
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.
Should we put the full URL into double quotes to make sure white space and other special characters don't break the command?
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.
Still working on this.
Need to debug the quotes, maybe add escape chars somewhere.
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 removed the user:pass from the URI and explicitly set it with -u. Including quotes to handle spaces.
Hostname and port can't have spaces, unless there is an edge case I'm missing.
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.
utACK, LGTM 🎉
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.
LGTM 🚀
Added the following env vars in the set_default_graf_dash.sh script, while keeping the default values in case they're not set:
This is very helpful when deploying with Docker or in a Kubernetes cluster. You can set the env vars in the docker-compose file, or your kubernetes yaml or helm charts.