search

Frzk / ansible-role-chrony

Ansible role to manage chrony.
Apache License 2.0
14 stars 8 forks source link

support other operation systems #5

Closed bodsch closed 3 years ago

bodsch commented 3 years ago

The PullRequest adds support for multiple operating systems and simplifies the handling of static variables under vars.

Unfortunately I couldn't get the GitHub workflow to work. :(

Frzk commented 3 years ago

Hi,

First, thanks a lot for your interest and work :-)

There's quote a lot going on here for a single PR, so I'll try to keep things clear.

.ansible-lint: nothing new here.

.editorconfig: not sure about this one, especially since it's probably lacking coherence with the linters config (line-length for example).

.flake8: I'm more encline to keep things per default and not ignore some rules for whatever reason.

.gitignore: not sure about this one either. The first line of my .gitignore file is .gitignore. I like that everyone deals with this personally. For example, I have a bunch of things in a global .gitignore file that won't appear in the project .gitignore. Also I don't see how to cope with every IDE out there creating temporary files, lock files, and so on. I know it's questionnable :-)

.yamllint: Shouldn't we check the yaml files in molecule, tests, ... ?

defaults/main.yml: mainly some whitespaces preferences. Mmkay.

handlers/main.yml: also a matter of preference I guess. For now I won't change this, but it would be a good idea to see if there is some convention or commonly-accepted style-guide regarding this. If so I'll be glad to accept a PR to correct this.

molecule/default/converge.yml: I need to have a more in-depth look at it.

molecule/default/molecule.yml: Quite a lot in here.

molecule/default/tests/*.py: I need to have a more in-depth look at it.

tasks/*.yml: I don't see any benefit in splitting the tasks into several small files. Can you elaborate on this ?

templates/chrony.conf.j2: Nope, using the comment filter is the way to go.

vars/main.yml: It seems like your re-inventing the wheel here :-) Ansible already does such a thing for you. Also, my method allows finer OS selection because it doesn't only look at ansible_os_family. Sorry, that's a no go for me.

I've started to work on testing and switching to GitHub workflow (mainly because of the Travis CI thing) but couldn't find the time to finish all this. This is clearly something that's on my to-do list. Just need to find some time.

Regarding your PR, I guess you see I can't accept this. I really hope it's not too "disappointing", I really do appreciate your proposals. It just don't think it'd fit for the most part. I'm open to discuss some points if you want :-)

Thanks again.

Frzk commented 3 years ago

Allright I'm closing this for reasons that seem pretty obvious to me. Please feel free to re-open if necessary :-) Thanks again.