saltstack-formulas / ntp-formula

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

Make sure ntp service is running and watching conf #43

Closed xenadmin closed 4 years ago

xenadmin commented 4 years ago

The state was installing ntp, but not making sure it runs. Furthermore I noticed that when I made changes to the used NTP servers, it wouldn't be used, because the service never watches/restarts. I tested that a few times and it fits my needs. I hope other folks will like it, too.

daks commented 4 years ago

it looks like this is handled in server.sls https://github.com/saltstack-formulas/ntp-formula/blob/995d0d74c49a607c11125cb9b84aeba4418f9cde/ntp/server.sls#L18

aboe76 commented 4 years ago

@daks, this formula needs some loving too, when looking at how it's structure,

n-rodriguez commented 4 years ago

this formula needs some loving too, when looking at how it's structure,

I do agree :)

daks commented 4 years ago

@aboe76 yes for sure, but it has no relation with the proposed PR IMHO

xenadmin commented 4 years ago

@daks Could be, but that's not the use case if you just want a NTP CLIENT, which is my use case. I have the following in my states/top.sls (notice the missing ntp.server)

base:
  zabbix_proxy:
    - match: nodegroup
    - authorized_keys
    - salt.minion
    - ntp
    - ntp.ntpdate

Now my pillar/top.sls

base:
  '*':
    - salt.minion
    - ntp.client

Which results in NO restart from the NTP client if I change my pillar, as I don't use NTP server, hence my PR.

aboe76 commented 4 years ago

@xenadmin you are right, and somebody also had a PR similar to this one: #29 so I'm ok with merging this,

daks commented 4 years ago

@xenadmin you're right, and that's what I was missing, sorry. So I'm OK with merging this PR.

The needed love makes sense now because this service.running could be factorized in an external state included by both client and server.

myii commented 4 years ago

Thanks for the PR, @xenadmin -- merged.

@daks @aboe76 Thanks for the reviews.