community.zabbix
community.zabbix copied to clipboard
ansible-collections
Refactor zabbix proxy
SUMMARY
Continuing the refactor, this time for zabbix-proxy
This PR consists of a lot of iterative commits, they should all be (somewhat) self-explanatory, doing a "single self-contained" change and a rationale for it. If one of the changes are not to anyones liking, comment on it and we can either improve upon it or take it out.
This is a considerably big refactor of the zabbix_proxy role. I'm not expecting it to break for existing installations and whatever overrides have been provided. It's not intended to "fix" any issues, just clean up the tasks and make the experience somewhat more consistent.
Big changes include moving package installation out of the os_family tasks, which are otherwise responsible for setting up the repo. So the installation happens in main.yml, and the dependencies for db-variants are installed in the mysql/postgresql/sqlite3 task files.
Arguably the repo-setup should be it's own role, as it's repeated out for every single role.
ISSUE TYPE
- Refactor Pull Request
COMPONENT NAME
zabbix_proxy
ADDITIONAL INFORMATION
I'd like for this refactor to retain all its commits and messages, and not become a squash merge. One thing I sorely missed during this refactor was the ability to find a log as to why things were done in a certain way.
I think this is pretty complete. Not quite sure about the when: zabbix_proxy_database_creation | bool constructions. You kinda want them there if you're passing variables from the command line. I've brought some changes spotted here over to PR #1193 for zabbix-server.
I'll get to zabbix-web next.
Hmm, the CentOS one we can probably fix by forcing python3?
I'll have to do some digging when it comes to the ubuntu fails.
Apologies for the hold up, been busy trying to get my own actions-runner working so I can get these results myself, as currently I'm currently just testing against a few select platforms and using images generated from Dockerfiles, so we can atleast have python3 in centos7. (A platform we might just drop sometime after summer?)
I'm currently experimenting with running a kind kubernetes cluster, and the actions-runner-controller, and had some marginal success getting it to run atleast :sweat_smile:
I'll get back to this once I have gotten repeatable tests.
I've created a separate PR #1218 for some CI stuff, and this PR has been rebased on that. The geerlingguy centos7 image comes without python3, and we should all be using python3 going forward.
Now it turns out rocky and almalinux and centos have systemd container-images ready to go, which is perfect our type of testing. So we just use those and install python3-pip which is a good enough compromise to get just the bare minimum of usable python on a system.
The debian/ubuntu images need the systemd-sysv package in addition to have /sbin/init present.
These are brain-dead simple docker-images, with bare minimum requirements, and will probably be more up-to-date and reliable for us in the long run.
Otherwise this pretty much follows the same pattern as the zabbix-server refactor.
Not sure why you put the same CI changes in here. Won't say remove them but need to fix the conflict you have.
Not sure why you put the same CI changes in here. Won't say remove them but need to fix the conflict you have.
I needed the CI stuff in here to make the tests pass. Not quite sure how I managed to get the conflict there myself, normally the ci changes would be recognized as already in and get skipped.
You lost a good deal of "documentation" in the git logs when you merged in the ci branch, is there a reason for squashing the commits? (The logs might come in handy for future on-lookers if they ever wonder why things were done in a certain way.)
As a rule we always squish commits. Most PRs have a fair number of meaningless commits to them as someone tries to figure out why something isn't working right (I'm guilty of this) or whatever so just to keep things as clean as possible we squish. You'll see that is the came with most of the community collections (or at least the one's I've spent time looking at.
As a rule we always squish commits.
Can you make an exception for these PRs? As you see there's quite a bit of documentation in each commit. And if they contain errors or need changes, I go back and fix them up instead of adding gibberish commits at the end of the PR.
OK so just going back through and reading all of the comments, I think my only concern right now is the Centos 7 python3 stuff and my comment about the valid version variable. I would suggest you just remove the centos 7 changes for right now. Unfortunately there's no getting around it being a breaking change until we remove Centos 7 completely. I really expected Zabbix 7 to be out by now based on their roadmap but that obviously isn't happening. Ref the valid version, lets just keep it in here and we can do a separate PR to do the rest of the roles. Thoughts?
Went through commit by commit and read your logic pulled out a few things. @BGmot and @D3DeFi what are your thoughts on not squishing commits?
Also forgot to leave another comment but missing change fragments.
@pyrodie18 Hi, I think that historically, it was required to squash commits when modules were a part of upstream ansible. Now I am not sure that there is any reason or preference other than those of maintainers of community repos.
My 2 cents are that squashing should be default unless there is good reason to merge all commits from PR without squashing them.
