search

saltstack-formulas / zabbix-formula

http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
50 stars 121 forks source link

Fix `salt-lint` violations => implement `semantic-release` inc. CI #129

Closed myii closed 4 years ago

myii commented 4 years ago

Having an initial look at what would be required to get semantic-release running for this formula. The whole package involves introducing multiple linters to check the code during the Travis run. The current codebase (at 96b653d) has a large number of salt-lint violations:

Even if semantic-release takes a while to be introduced to this formula, these violations really need to be resolved.

myii commented 4 years ago

@hatifnatt @xenadmin Actually, this formula is in pretty ripe for conversion. Setting version_repo: '4.4' for installation, we've got a nice set of passes already:

That's based on the formula-specific parts of top.sls as suggested in the README, i.e.:

      state_top:
        base:
          '*':
            - zabbix.agent.repo
            - zabbix.agent.conf
            - zabbix.server.repo
            - zabbix.server.conf
            - zabbix.frontend.repo
            - zabbix.frontend.conf

If you're OK with the tabs being changed to spaces, we could have this rolled out pretty soon.

hatifnatt commented 4 years ago

I don't have numbers how widely various OS-es and specific versions are used on servers, personally I'm using mostly Debian and sometimes Ubuntu so I can vote for Debian 10. I'm totally Ok about switching to spaces in yaml, sls ans jinja files which are not templates. But I'm strongly against converting tabs to spaces in template files, tabs there for reason :) Same with a long lines, I see no reason to put effort in removing long lines from templates, long lines in current formula templates are used for easier comparison in a diff tools like Meld, etc.

myii commented 4 years ago

I don't have numbers how widely various OS-es and specific versions are used on servers, personally I'm using mostly Debian and sometimes Ubuntu so I can vote for Debian 10.

We're still in the process of getting Debian 10 rolled out in our CI setup. There are so many combinations to cater for that we can only make a best effort. To give you an idea of the current situation, have a look at this wiki page:

... But I'm strongly against converting tabs to spaces in template files, tabs there for reason :)

And that's why I asked, due to your familiarity with this formula. In other formulas where the linters are tripped, we can add them to the ignore list.

Same with a long lines, I see no reason to put effort in removing long lines from templates, long lines in current formula templates are used for easier comparison in a diff tools like Meld, etc.

That's where I disagree with you. Long lines are nearly always poor for comprehension and maintainability. It's hard for new contributors to get their heads around what is going on. The diffs in GitHub are poor. It indicates that the underlying data structures can be improved. Even simply making better use of Jinja variables can resolve a lot of these problems.

In this case, we can also add zabbix/files/default/etc/zabbix/zabbix_proxy.conf.jinja to the ignores list, so it won't be a problem.

With that all said, shall we start with what we've got? We can always improve things over time. It actually happens, since we ensure that common updates are propagated to all of the semantic-release formulas.

hatifnatt commented 4 years ago

I'm only talking about long lines in this formula templates, they can be shortened for sure, but I don't think it's worth it. So I can suggest simply add all templates to ignore list, at least until they will be improved in terms of line length. Personally, I do not mind to start.

saltstack-formulas-travis commented 4 years ago

:tada: This issue has been resolved in version 0.21.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: