Rework 4 #28
Rework 4 #28
Conversation
git ls-files -z "$(git rev-parse --show-toplevel)" | xargs --null -I '{}' find '{}' -type f -print0 | xargs --null sed --in-place --regexp-extended 's#checkmk_agent__hostname#checkmk_agent__fqdn#g;'
Don’t follow bad upstream examples.
|
Wow, thanks a lot for this update. Please give me some time to go through the changes before I approve. It looks like there is still an issue with a condition on a plugin. Maybe you still can have a look at that? |
|
Ah, I guess I was a bit too fast in responding. I guess the plugin issue is fixed in ganto/debops-test-suite#2? |
I have not tried it on Travis but that would be my guess that with this merged, CI should pass. |
There was a problem hiding this comment.
Thanks a lot for this PR. There is a lot of cleanup from the very early days of this role. 👍
I never used the automated plugin setup of this role, so I just hope the split into the checkmk_agent/env role is worth it. I'm usually not so much in favor of this strategy as it makes it more complicated to use this role without DebOps. Therefore it would be good to have it properly documented (see my other comment).
| "checkmk_agent__plugin_list" : {{ checkmk_agent__tpl_plugin_list | to_nice_json }} | ||
| } | ||
| {{ ({ | ||
| "plugins": (debops__tpl_macros.flattened(checkmk_agent__tpl_plugins) | from_json | unique), |
There was a problem hiding this comment.
Since there is a new "sub"-role for setting the facts, could you please still add some documentation in docs/ on the purpose/influence of the facts file?
There was a problem hiding this comment.
Done. I had it "documented" here:
# .. envvar:: checkmk_agent__combined_plugins [[[
#
# Combined list of all plugins which are going to be installed.
# Specified as Ansible local fact so that this variable is also valid in ``when``
# conditions evaluated in the context of other roles called from the same playbook as this role.I documented the purpose a bit more verbosely in docs/ now.
There was a problem hiding this comment.
Ok, something I forgot to mention: In my opinion it would make sense to introduce a page e.g. docs/facts.rst, where the format of the facts is described. As I started to discuss before in debops/debops-policy#16 (comment) it would be nice to have a clear and mostly stable reference about the individual role facts so that a any 3rd-party tool evaluating the facts (e.g. asset inventory tool or such) can reason about the configuration of the role without forcing the author to read and understand the facts template. What do you think?
There was a problem hiding this comment.
Sounds good to me. Do you have a example/template of such a stable reference I can use?
There was a problem hiding this comment.
Not yet. What I think it should contain:
- Name of the facts key (below
ansible_local). Most of the time that's the role name, but sometimes also not 😉 - Each key which can be there and if it's always there or optional.
- Format and short description of the according key values. I guess most of the time this will be just a reference to a variable from
defaults.
I guess the formatting is good enough to be like e.g. the docs/defaults-detailed.rst.
There was a problem hiding this comment.
Good idea. I added a first draft which can be updated when a documentation format has been specified.
vars/main.yml
Outdated
|
|
||
| # Default WATO host attributes. This variable, when being merged with | ||
| # `checkmk_agent__host_attributes`, allows to successfully compare the WebAPI | ||
| # get_host() response in case the `contactgroups` key is set. |
There was a problem hiding this comment.
Hmn, not so happy that you remove this from here. This is something that for developer convenience and readability has been separated into an own (internal) variable. However, this must not be changed by the user. That's why I tried to "hide" it in the vars file.
There was a problem hiding this comment.
This is mainly done for transparency reasons. checkmk_agent__default_host_attributes is later combined with checkmk_agent__host_attributes. Ideally, the checkmk_agent__fact_host_attributes could also be moved to defaults. Would you be ok with this or should I change it back to vars?
There was a problem hiding this comment.
I see your point. But so far this is something that has many edge cases I still need to look into, so I'd prefer that you don't touch it.
The format of the server facts will change with debops-contrib/ansible-checkmk_server#53 anyway, so I'll have a look at the autojoin feature again in the foreseeable future. Then I'll make sure to clean it up and move it to defaults.
|
Incorporated your review feedback and CI is passing. |
Reworks: #6
I dropped the attempt to run the agent under a non-root user.