feat(agent_conf) update zabbix agent formula to v7 #177
Conversation
Adding support for version 7 to zabbix agent Change zabbix URL to https
There was a problem hiding this comment.
You've done a lot of work to bring the template to match with Zabbix Agent v7 configuration file, thank you.
I probably understand why you want to remove minus symbol - you want every option to be surrounded by empty lines, and this will trigger lots of changes for every user of this formula. But introducing empty lines in this case can't be considered as "substantial" change.
I personally don't think this change is required at all, but if you really want to tinker with empty lines, please make separate "refactoring" PR.
*May be even with BREAKING label for people to pay extra attention to this changes, although they are not really will break anything.
Please also pay attention to a few other issues that I have noticed.
| LogFile={{ settings.get('logfile', defaults.logfile) }} | ||
| {% endif %} | ||
| {% else -%} | ||
| {% else %} | ||
| ### Option: LogFile |
There was a problem hiding this comment.
As you can see there is only one empty line after LogFile= currently {% endif %} will create one empty line and {% else -%} wont create empty lines, when you remove minus symbol from {% else -%} 2 empty lines will be added. That's a concrete example why minus symbol is required.
Also need to mention this part of the file have not been changed since 2009
https://git.zabbix.com/projects/ZBX/repos/zabbix/commits/075fa7f46b5341da547fdde686de47466a8ee805#conf/zabbix_agentd.conf so it's very unlikely that changes to the template file are required.
Overall removing minus symbol in many many places is not directly related with updating template to be compatible with Zabbix v7. Moreover this unnecessary changes are shading important changes, it's hard for me to understand which changes are substantial, and which are just tinkering with a new lines.
Concluding all above please don't remove minus signs.
There was a problem hiding this comment.
I was only trying to match default file from zabbix... If it causes an issue, i will re-add the minus sign...
However...
In the existing template, there are inconsistencies in the white space removal... eg: line 16, introduce whitespace, line 29, remove whitespace, line 38 , introduce whitespace again....
Do you want it uniform across the template, or should i leave existing template as is and only introduce the new changes?
There was a problem hiding this comment.
I understand what are you trying to achieve but best practice is not to mix new features and refactoring in a one commit / PR. If you want to change something in the area of style or formatting it can be called "refactoring" even if it's "just few new lines" and it must be done in a separate PR.
Yes it's not 100% consistent with empty lines in this template but if you add / remove empty lines every user of this formula will get huge diff after updating the formula to the new version and applying it with the existing configuration. And they will be forced to dive deep into this diff just to find that it's all about empty lines and whitespaces and not something significant.
I myself am very much inclined to keep things straight and consistent, but in these cases you just have to let go. Empty lines won't add any significant substance.
So, please, only new changes in this PR.
| {% if settings.get('hostnameitem', defaults.get('hostnameitem', False)) -%} | ||
| {% if settings.get('hostnameitem', defaults.get('hostnameitem', False)) %} | ||
| HostnameItem={{ settings.get('hostnameitem', defaults.hostnameitem) }} | ||
| {% endif %} |
There was a problem hiding this comment.
You have removed conditional for hostnameitem that's way Hostname= param will always be added to the configuration file and HostnameItem= will never have any effect
Read description for HostnameItem
Option: HostnameItem
Item used for generating Hostname if it is undefined. Ignored if Hostname is defined.
Does not support UserParameters or aliases.
There was a problem hiding this comment.
Mistake - Thanks for spotting.
| {% if zabbix.version_repo|float >= 4.4 -%} | ||
| ### Option: HostInterface | ||
| # Optional parameter that defines host interface. | ||
| # Host interface is used at host auto-registration process. | ||
| # An agent will issue an error and not start if the value is over limit of 255 characters. | ||
| # If not defined, value will be acquired from HostInterfaceItem. | ||
| # | ||
| # Mandatory: no | ||
| # Range: 0-255 characters | ||
| # Default: | ||
| # HostInterface= | ||
| {% if settings.get('hostinterface', defaults.get('hostinterface', False)) -%} | ||
| HostInterface={{ settings.get('hostinterface', defaults.hostinterface) }} | ||
| {% endif %} | ||
| ### Option: HostInterfaceItem | ||
| # Optional parameter that defines an item used for getting host interface. | ||
| # Host interface is used at host auto-registration process. | ||
| # During an auto-registration request an agent will log a warning message if | ||
| # the value returned by specified item is over limit of 255 characters. | ||
| # This option is only used when HostInterface is not defined. | ||
| # | ||
| # Mandatory: no | ||
| # Default: | ||
| # HostInterfaceItem= | ||
| {% if settings.get('hostinterfaceitem', defaults.get('hostinterfaceitem', False)) -%} | ||
| HostInterfaceItem={{ settings.get('hostinterfaceitem', defaults.hostinterfaceitem) }} | ||
| {% endif %} | ||
| {% endif -%} |
There was a problem hiding this comment.
Why was this removed? It's still present in Zabbix Agent v7 configuration.
There was a problem hiding this comment.
Mistake - Thanks for spotting.
| LogFile={{ settings.get('logfile', defaults.logfile) }} | ||
| {% endif %} | ||
| {% else -%} | ||
| {% else %} | ||
| ### Option: LogFile |
There was a problem hiding this comment.
I understand what are you trying to achieve but best practice is not to mix new features and refactoring in a one commit / PR. If you want to change something in the area of style or formatting it can be called "refactoring" even if it's "just few new lines" and it must be done in a separate PR.
Yes it's not 100% consistent with empty lines in this template but if you add / remove empty lines every user of this formula will get huge diff after updating the formula to the new version and applying it with the existing configuration. And they will be forced to dive deep into this diff just to find that it's all about empty lines and whitespaces and not something significant.
I myself am very much inclined to keep things straight and consistent, but in these cases you just have to let go. Empty lines won't add any significant substance.
So, please, only new changes in this PR.
2 participants
Adding support for version 7 to zabbix agent
Change zabbix URL to https
PR progress checklist (to be filled in by reviewers)
What type of PR is this?
Primary type
[build]Changes related to the build system[chore]Changes to the build process or auxiliary tools and libraries such as documentation generation[ci]Changes to the continuous integration configuration[feat]A new feature[fix]A bug fix[perf]A code change that improves performance[refactor]A code change that neither fixes a bug nor adds a feature[revert]A change used to revert a previous commit[style]Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)Does this PR introduce a
BREAKING CHANGE?No.
Describe the changes you're proposing
Adding support for version 7 to zabbix agent
Documentation checklist
README(e.g.Available states).pillar.example.Testing checklist
state_top).Additional context