-
Notifications
You must be signed in to change notification settings - Fork 27
Adds script to import dashboards to terraform #73
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
Adds script to import dashboards to terraform #73
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the Code of Conduct and I hereby accept the Terms |
|
Wow thank you for contributing this!! I'm going to give it a once over test then we can get this merged. Again thank you @jinja2 🖖 |
greatestusername-splunk
left a comment
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.
A couple comments on the docs otherwise lgtm! Thank you so much for contributing this!
| ```sh | ||
| cd ${REPO_ROOT} | ||
| docker run -v $(pwd):/app import-tf-script \ | ||
| --auth-token <TOKEN> \ |
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.
Mind taking another look at this block?
Does group-ids Does it need a list [id1,id2,id3] or id1,id2,id3?
<id1,id2> does not appear to be valid syntax
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.
Looks like this has some variable names that need changing. This worked:
docker run -v $(pwd):/main import-tf-script \
--api-token mytokentokentoken \
--api-url https://api.us1.signalfx.com \
--groups GJsIuOtA4Ag \
--dir /beep
so --groups instead of --group-ids and --api-token instead of auth-token
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! I have fixed the readme.
| For a quick start cmd to generate terraform code for dashboard group, run the following make command: | ||
|
|
||
| ```sh | ||
| SIGNALFX_AUTH_TOKEN=<SIGNALFX API TOKEN> GROUP_IDS=<ID1,ID2> RELATIVE_DIR_PATH=resources/Synthetics-Dashboards make import-dashboard-group |
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.
maybe add SIGNALFX_API_URL here for clarity in case folks not on us1 use this?
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.
done
| docker buildx build -t $(IMAGE_NAME) . | ||
|
|
||
| .PHONY: import-dashboard-group | ||
| import-dashboard-group: clean build |
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.
May be worth listing out in the readme what env vars users have available for setting things like their url (realm) and token and so forth?
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.
Added a section for this
|
Could we possibly list in the README what chart types are NOT supported? I imagine log query charts but to your knowledge is anything else not supported? |
|
Can this PR get some love? Content looks very usefull, what is needed to move forward? |
|
Ah, forgot about this PR. I'll try to address the comments when I have some free time to move this forward. |
3bffd91 to
d93d1d8
Compare
|
@greatestusername-splunk thanks for the review! I have made the requested changes to the readme and updated pkg versions. |
Oh thank you! Will take a look today. Really appreciate you circling back!! |
greatestusername-splunk
left a comment
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.
TY for adding these changes and updates! Much appreciated!!
This PR is to add a tool which can enable users to import existing SignalFx dashboards to Terraform.