Modernization rewrite #16
Conversation
Install chart support if configured, false by default
Repos and chart support for OpenSUSE
Chart support
Linting fixes
Fix line length
The latter is up to upstream to fix
Install recommends on all platforms
manage netdata config when netdata_custom_config_path is also not empty
| @@ -1,2 +1,3 @@ | |||
| # For the time being, @Ferroin is responsible for everything here. | |||
| # For the time being, @Ferroin is responsible for everything here | |||
There was a problem hiding this comment.
This is probably an accidental change.
| .vscode | ||
| hosts | ||
| ansible.cfg | ||
| .swp |
There was a problem hiding this comment.
I have an ambivalent approach towards .gitignore files in repos, generally opting for a combination of a well maintained ~/.config/git/ignore and project specifc .git/info/exclude in the working tree. On the other hand, our Agent repo has a very extensive one.
That said, if you want to exclude ViM (swap) files, have a look at https://github.com/github/gitignore/blob/main/Global/Vim.gitignore.
There was a problem hiding this comment.
I'm equivalently ambivalent about it. This was mainly for my convenience. I'll remove it.
| # Example of basic Netdata agent management using Ansible | ||
| ## Prerequisites | ||
| Tested with Ansible v. 2.12.1; should work with any Ansible version since 2.9 | ||
| # Netdata Ansible |
There was a problem hiding this comment.
At some point, possibly not as part of this PR, but definitely before publishing this to Galaxy, we need to expand this (and also https://learn.netdata.cloud/) with all the current tweakables and expand example usage.
| @@ -0,0 +1,43 @@ | |||
| --- | |||
| role_version: 1.0.0 | |||
There was a problem hiding this comment.
Possibly we should have 0.x.x here. Let's have 1.0.0 when we do an actual release. I also don't think that information goes here, but in meta/main.yml in a galaxy_info object, or in galaxy.yml?
| role_version: 1.0.0 | ||
|
|
||
| # Define the Netdata release version we install | ||
| netdata_release_version: "stable" |
There was a problem hiding this comment.
We call this release channel.
Also, the names we typically use are stable and nightly, but the the native build repos use edge instead of nightly. I think it would be good so be able to put nightly as a value here and it doing the right thing.
There was a problem hiding this comment.
@ralphm I'll update this to reflect the release channel terminology.
As for nightly versus stable, I'd suggest considering the target audience for the Ansible versus the command-line installer. The script defaulting to nightly is unexpected and surprising as a user, and I'd certainly be displeased as a sysadmin if Ansible auto-installed a nightly/non-stable version by default.
Happy to be convinced otherwise here.
There was a problem hiding this comment.
Oh, I did not mean having nightly as the default. Just that it can take that as a value, instead of, or next to edge.
| owner: root | ||
| group: root | ||
| mode: '0644' | ||
| notify: Restart Netdata |
There was a problem hiding this comment.
We don't really need to restart the Agent in order for it to process claim.conf. The Netdata CLI has a reload-claiming-state command that calls a corresponding API in the Agent to just reload its config and (re-)connect to Cloud.
It might be nice to prevent restarting Netdata if we don't need to because of other notify calls.
There was a problem hiding this comment.
Ah nice, I didn't realize that occurred. I'll remove the restart.
| {% if netdata_proxy %} | ||
| proxy = {{ netdata_proxy }} | ||
| {% endif %} | ||
| insecure = 'no' |
There was a problem hiding this comment.
Given that no is the default and we are not making this into a variable, we can do two things:
- Just remove it from the template.
- Have some kind of dict for other options, like
insecure, providing a way to control it.
I'm favoring option 1 right now.
|
|
||
| - name: Lay down charts.d.conf | ||
| ansible.builtin.copy: | ||
| content: 'enable_all_charts="yes"' |
There was a problem hiding this comment.
This is the default. Why make this explicit, while not offering any other configuration?
There was a problem hiding this comment.
Ah, is this enabled by default without charts.d.conf existing?
| @@ -0,0 +1,37 @@ | |||
| --- | |||
There was a problem hiding this comment.
This is identical to install-debian.yml. Can we consolidate, e.g. by using ansible_pkg_mgr?
| @@ -0,0 +1,2 @@ | |||
| --- | |||
| # vars file for role | |||
There was a problem hiding this comment.
I think we can remove this, if empty.
|
|
|
@agomerz : Can you please sign the CLA? |
5 participants
This is an attempt to modernize the Netdata Ansible.
This currently supports Debian, Ubuntu, and OpenSUSE, but additional distributions can be supported easily and in a consistent pattern.
Patches welcome.
Fixes #10
Fixes #7
Fixes #12 via deprecation, though we should handle binary installs gracefully.
Deprecates #8 - I would like to see Molecule tests re-added in the future.