zabbix_short_version tests are sometimes testing integer, sometimes testing strings

[clement-lefevre]

There are several parts in the repository, especially in templates/zabbix_server.conf.j2 with mixed tests, testing both string and integer along the file.

In YAML files, tests are stating zabbix_short_version is an integer while it is a string by construction: zabbix_short_version: "{{ zabbix_version | regex_replace('\\.', '') }}"

This leads to this kind of fail:

fatal:foobar: FAILED! => {"failed": true, "msg": "The conditional check 'zabbix_short_version < 30' failed. The error was: Unexpected templating type error occurred on ({% if zabbix_s
hort_version < 30 %} True {% else %} False {% endif %}): '<' not supported between instances of 'str' and 'int'\n\nThe error appears to have been in '/foo/bar/ansible/roles/zabbix-serv
er/tasks/Debian.yml': line 2, column 3, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n---\n- name: \"Debian | Set some facts\"\n  ^ here\n"} 

About this issue, and compared to other versions, what was the need to create this zabbix_short_version (why short by the way?) cutting out the dot in the version? Or setting this directly if this is really needed, as it looks like zabbix_version is only used to create zabbix_short_version?

EDIT: It looks like Zabbix 3.4 is now out. Repository might need an update to add support for it in the same time on some of those tests. But this might need another issue/PR.

[johackim]

:+1:

[dj-wasabi]

Hi, the github master branch is not using the zabbix_short_version anymore, it is using the version_compare filter now.

Example:

{% if zabbix_version | version_compare('3.2', '<=') %}

[clement-lefevre]

(Sorry, reposting, wrong account :-) )

Yeah, I saw that. I don't know what's the added value though. I had to fix the master for my own usage looking at how it was when I cloned, and I replaced those with zabbix_version being a float X.Y and simple tests like zabbix_version > 3.0 or ansible_version <= 12.04 and it was working flawlessly.

I didn't fully read the current master because it seems a lot of things were rewritten, previous tests were not considering Zabbix 3.4 and Debian 9 Stretch, so in could be necessary to add them (that's another issue though)

[dj-wasabi]

Hi @clement-lefevre

It see the version_compare a lot lately, so as this seems normal in most of the roles.

What do you mean with this previous tests were not considering Zabbix 3.4 and Debian 9 Stretch, so in could be necessary to add them (that's another issue though) part? Are more tests needed? If so, what kinds of tests should be added?

[clement-lefevre]

Well, if you're more cumfortable with version_compare, now that curly braces are gone, I guess that's fine.

My statements were pre-1.0 merge, as I told you I didn't went a lot through the 1.0 as of now, and might not that much as I won't work with anymore anymore soon. Most of the fixes I did pre-1.0 are not needed anymore including tests as those were fixed. Only #68 remains.

pre-1.0, Zabbix 3.4 was not considered nor mentionned. I now see that it's here in README, so I guess all works well with it. For Debian 9, I don't know. It's not mentionned in the README, but I can't test if all is fine with it, and looking at how you refactored everything, I'd say it looks fine now.

So, I guess this issue is now pretty deprecated and uneeded (ie. can be closed)? :)

[dj-wasabi]

Ok. Thank you for your explanation. I'll close the issue. If you do find something don't hesitate to create an issue or an pull request! 👍