salt 2019.2 yaml render compatibility

[dimabutyrin]

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]

@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]

@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]

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 ***@***.***> 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 <#41 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGuxOx5fLfNjWthdk00Vuk_egTS09Jkbks5vaj65gaJpZM4cLf23> .

[myii]

@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]

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

[myii]

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

[aboe76]

@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]

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

[dimabutyrin]

@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]

@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]

@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]

@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]

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

[aboe76]

merged it