search

saltstack-formulas / ntp-formula

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

salt 2019.2 yaml render compatibility #41

Closed dimabutyrin closed 5 years ago

dimabutyrin commented 5 years ago

Salt 2019.2.0 has Non-Backward-Compatible Change to YAML Renderer, more here: https://docs.saltstack.com/en/latest/topics/releases/2019.2.0.html#non-backward-compatible-change-to-yaml-renderer

Without these changes ntp.ng state is failing:

$ salt-call --local state.apply ntp.ng
[CRITICAL] Rendering SLS 'base:ntp.ng' failed: found unexpected ':'
local:
    Data failed to compile:
----------
    Rendering SLS 'base:ntp.ng' failed: found unexpected ':'
$ salt-call -V
Salt Version:
           Salt: 2019.2.0

Dependency Versions:
           cffi: 1.12.2
       cherrypy: Not Installed
       dateutil: 1.5
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.7.2
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: 0.9.1
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: 1.2.3
      pycparser: 2.19
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.6 (default, Nov 13 2018, 12:45:42)
   python-gnupg: Not Installed
         PyYAML: 3.10
          PyZMQ: 14.0.1
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.0.5

System Versions:
           dist: Ubuntu 14.04 trusty
         locale: ANSI_X3.4-1968
        machine: x86_64
        release: 3.13.0-167-generic
         system: Linux
        version: Ubuntu 14.04 trusty
pillar.sls
# used in the ntp formula https://github.com/saltstack-formulas/ntp-formula
ntp:
  ng:
    settings:
      ntpd: True
      ntp_conf:
        server: ['0.us.pool.ntp.org', '1.us.pool.ntp.org']
        restrict: ['127.0.0.1', '::1']
        driftfile: ['/var/lib/ntp/ntp.drift']
myii commented 5 years ago

@dimabutyrin Thanks for this. Due to long discussion about supporting older versions of Salt, we currently need to use the | json renderer instead of the | tojson filter.

myii commented 5 years ago

@dimabutyrin Thanks for the update. Now as I've gone to test it, I can't find a problem before this fix. I've tried with and without a pillar (using pillar.example). Can you share a pillar configuration that causes this breakage?

dimabutyrin commented 5 years ago

Its attached to the initial PR message. In the bottom. Along with the salt version-report.

On Tue, Mar 26, 2019, 5:37 PM Imran Iqbal notifications@github.com wrote:

@dimabutyrin https://github.com/dimabutyrin Thanks for the update. Now as I've gone to test it, I can't find a problem before this fix. I've tried with and without a pillar (using pillar.example). Can you share a pillar configuration that causes this breakage?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/saltstack-formulas/ntp-formula/pull/41#issuecomment-476707690, or mute the thread https://github.com/notifications/unsubscribe-auth/AGuxOx5fLfNjWthdk00Vuk_egTS09Jkbks5vaj65gaJpZM4cLf23 .

myii commented 5 years ago

@dimabutyrin Of course it is! Really sorry, I'm doing multiple tasks at the same time so I missed that. However, I've just used that exact pillar with an Ubuntu 14.04 2019.2 minion and I can't reproduce this. To be fair, adding | json still works, so it could still be merged to support edge cases. However, it does affect the ordering rendered in the file, so there will be false positives for existing users, thinking the configurations have changed, when they actually haven't.

@aboe76 Would you mind looking in to this?

dimabutyrin commented 5 years ago

@myii are you checking ntp.ng state? ntp state works fine.

myii commented 5 years ago

@dimabutyrin Absolutely, ntp.ng in all of my tests, with and without the master.

aboe76 commented 5 years ago

@dimabutyrin Of course it is! Really sorry, I'm doing multiple tasks at the same time so I missed that. However, I've just used that exact pillar with an Ubuntu 14.04 2019.2 minion and I can't reproduce this. To be fair, adding | json still works, so it could still be merged to support edge cases. However, it does affect the ordering rendered in the file, so there will be false positives for existing users, thinking the configurations have changed, when they actually haven't.

@aboe76 Would you mind looking in to this?

@myii the ntp.conf file order isn't a problem for the functioning of ntp, only cosmetic. I see no issue with this PR.

myii commented 5 years ago

@aboe76 That's fine, I've got no blocks to this being merged. One question: are you able to reproduce the original issue?

dimabutyrin commented 5 years ago

@myii today I checked it in a production setup, that have master-minion deployment. And the issue is not reproducible. I guess it's something wrong with salt-call, then?

myii commented 5 years ago

@dimabutyrin It's probably something to do with salt-call but I did try that at my end and it still worked. I don't have a pure standalone minion to test with, so that's probably why I can't reproduce it. What are your thoughts about this, @aboe76?

aboe76 commented 5 years ago

@myii sorry I use the chrony-formula and systemd-formula for timekeeping, haven't tried ntp-formula for a while let me it up again on a minion.

aboe76 commented 5 years ago

@dimabutyrin @myii can't reproduce, I have upgraded al my minions to a py3 version...

But having this in here isn't doing any harm...and if it solves a edge case that's nice.

myii commented 5 years ago

@aboe76 I'm fine either way, I'll leave it to you to make the final decision.

aboe76 commented 5 years ago

merged it