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
nixos/grafana: add provisioning of pre-configured Grafana with dashb… #54561
Conversation
…ard and datasource
Is there something to change here or add? |
''; | ||
}; | ||
|
||
dashboardConfigText = mkOption { |
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.
Why are there json and yaml configs? Do they serve the same purpose? If so, there's no need to allow config for both of them, only the yaml one will be enough.
or replace to double quote, because it interfere with Nix syntax! | ||
Example: | ||
|
||
apiVersion: 1 |
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.
Examples should go as an example =
attribute, and for this you'll need literalExample
(see nixpkgs for examples)
Dashboard configuration as YAML text. If non-null, this option | ||
defines the text that is written to dashboard_conf.yml. | ||
Avoid two apostrophes in a row inside YAML as empty value | ||
or replace to double quote, because it interfere with Nix 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.
No need to point this out, people should be familiar with Nix syntax and not need a reminder in the options.
@@ -78,6 +84,64 @@ in { | |||
type = types.str; | |||
}; | |||
|
|||
datasourceConfigText = mkOption { |
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.
Same feedback as for dashboardConfigText
email = "dmitriy.lifanov1@gmail.com"; | ||
github = "dlifanov"; | ||
name = "Dmitriy Lifanov"; | ||
}; |
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.
You should put this change into a separate commit (so this PR has 2 commits)
mkdir -p ${cfg.dataDir}/provisioning/{dashboards,datasources} | ||
cat ${datasourceConfig} > ${cfg.dataDir}/provisioning/datasources/datasource_conf.yml | ||
cat ${dashboardConfig} > ${cfg.dataDir}/provisioning/dashboards/dashboard_conf.yml | ||
cat ${dashboardFile} > ${cfg.dataDir}/provisioning/dashboards/dashboard.json |
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.
Useless use of cat, you can just ln -s
these. Also, doesn't this break backwards compatibility for people that had these files specified already?
That one just points to dashboards path. The dashboard will consist of over hundred variables not viable for separate options. Here dashboard changes are under version control and it is deployed with Grafana. |
I'm sorry that I wasn't clear enough: the datasource can be configured declaratively (which isn't that complex, right?) which is why I prefer it. The PR I pointed to also links to the dashboard config file rather than implementing tons of options. In that case you're absolutely right, implementing dashboard config entirely in Nix wouldn't be appropriate. |
@dlifanov @infinisil @mkaito FYI, #53874 seems ready. Unless there are any reasons I'm missing why this PR should be preferred, I'd merge it this afternoon. |
I merged #53874 now because of the the reasons I expressed earlier. |
Closing for now as it's superseded by #53874, feel free to reopen or ping me if needed. |
Motivation for this change
Grafana is deployed without pre-configured dashboard and datasource.
Add configuration to get already working dashboard.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)