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

nixos/grafana: add provisioning of pre-configured Grafana with dashb… #54561

Closed

Conversation

@dlifanov
Copy link

@dlifanov dlifanov commented Jan 24, 2019

Motivation for this change

Grafana is deployed without pre-configured dashboard and datasource.
Add configuration to get already working dashboard.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@dlifanov
Copy link
Author

@dlifanov dlifanov commented Jan 30, 2019

Is there something to change here or add?

mkaito
mkaito approved these changes Feb 4, 2019
'';
};

dashboardConfigText = mkOption {
Copy link
Member

@Infinisil Infinisil Feb 4, 2019

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
Copy link
Member

@Infinisil Infinisil Feb 4, 2019

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!
Copy link
Member

@Infinisil Infinisil Feb 4, 2019

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 {
Copy link
Member

@Infinisil Infinisil Feb 4, 2019

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";
};
Copy link
Member

@Infinisil Infinisil Feb 4, 2019

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
Copy link
Member

@Infinisil Infinisil Feb 4, 2019

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?

@Ma27
Copy link
Member

@Ma27 Ma27 commented Feb 5, 2019

This is a duplicate to #53874. Also I prefer #53874 since it provides config options for NixOS, so no need to write tons of config in multiline strings.

@dlifanov
Copy link
Author

@dlifanov dlifanov commented Feb 5, 2019

This is a duplicate to #53874. Also I prefer #53874 since it provides config options for NixOS, so no need to write tons of config in multiline strings.

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.

@Ma27
Copy link
Member

@Ma27 Ma27 commented Feb 5, 2019

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.

@Ma27
Copy link
Member

@Ma27 Ma27 commented Feb 6, 2019

@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.

@Ma27
Copy link
Member

@Ma27 Ma27 commented Feb 6, 2019

I merged #53874 now because of the the reasons I expressed earlier.

@Ma27
Copy link
Member

@Ma27 Ma27 commented Feb 26, 2019

Closing for now as it's superseded by #53874, feel free to reopen or ping me if needed.

@Ma27 Ma27 closed this Feb 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants