Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Read Pillar files into OrderedDict to preserve source order #12161

Closed
westurner opened this issue Apr 21, 2014 · 17 comments
Closed

ENH: Read Pillar files into OrderedDict to preserve source order #12161

westurner opened this issue Apr 21, 2014 · 17 comments
Labels
Feature new functionality including changes to functionality and code refactors, etc. order
Milestone

Comments

@westurner
Copy link
Contributor

It would be great if .iteritems of pillar data read out in source order.

Example:

Pillar Input: https://github.com/saltstack-formulas/openssh-formula/blob/master/pillar.example

sshd_config:
  Port: 22
  Protocol: 2
  HostKey:
    - /etc/ssh/ssh_host_rsa_key
    - /etc/ssh/ssh_host_dsa_key
    - /etc/ssh/ssh_host_ecdsa_key
  UsePrivilegeSeparation: yes
  KeyRegenerationInterval: 3600
  ServerKeyBits: 768
  SyslogFacility: AUTH
  LogLevel: INFO
  LoginGraceTime: 120
  PermitRootLogin: yes
  StrictModes: yes
  RSAAuthentication: yes
  PubkeyAuthentication: yes
  IgnoreRhosts: yes
  RhostsRSAAuthentication: no
  HostbasedAuthentication: no
  PermitEmptyPasswords: no
  ChallengeResponseAuthentication: no
  X11Forwarding: yes
  X11DisplayOffset: 10
  PrintMotd: no
  PrintLastLog: yes
  TCPKeepAlive: yes
  AcceptEnv: "LANG LC_*"
  Subsystem: "sftp /usr/lib/openssh/sftp-server"
  UsePAM: yes

Template: https://github.com/saltstack-formulas/openssh-formula/blob/master/openssh/files/sshd_config

{% set sshd_config = pillar.get('sshd_config', {}) %}
# {...}
{% for keyword, argument in sshd_config.iteritems() %}
    {%- if argument is sameas true %}
{{ keyword }} yes
    {%- elif argument is sameas false %}
{{ keyword }} no
    {%- elif argument is string or argument is number %}
{{ keyword }} {{ argument }}
    {%- else %}
        {%- for item in argument %}
{{ keyword }} {{ item }}
        {%- endfor %}
    {%- endif %}
{%- endfor %}
# {...}

Output: /etc/ssh/sshd_config

StrictModes yes
IgnoreRhosts yes
PermitEmptyPasswords no
PermitRootLogin no
UseDNS no
HostKey /etc/ssh/ssh_host_rsa_key
HostKey /etc/ssh/ssh_host_dsa_key
HostKey /etc/ssh/ssh_host_ecdsa_key
LogLevel INFO
X11DisplayOffset 10
AcceptEnv LANG LC_*
TCPKeepAlive yes
X11Forwarding yes
KeyRegenerationInterval 3600
Subsystem sftp /usr/lib/openssh/sftp-server
ServerKeyBits 768
UsePrivilegeSeparation yes
SyslogFacility AUTH
RhostsRSAAuthentication no
PrintLastLog yes
ChallengeResponseAuthentication no
LoginGraceTime 120
PubkeyAuthentication yes
UsePAM yes
Protocol 2
PrintMotd no
HostbasedAuthentication no
RSAAuthentication yes
Port 22
@basepi
Copy link
Contributor

basepi commented Apr 21, 2014

As far as I know, we are not using OrderedDicts for basic template data, only for the topfile for pillar data. We'll investigate using them for all pillar data to get ordered iteritems.

@basepi basepi added the Feature label Apr 21, 2014
@basepi basepi added this to the Approved milestone Apr 21, 2014
@westurner westurner changed the title Read Pillar files into OrderedDict to preserve source order ENH: Read Pillar files into OrderedDict to preserve source order Aug 20, 2014
@aboe76
Copy link
Contributor

aboe76 commented Feb 13, 2015

I got almost the same kind of issues with my own formula of h2o web server:
where file.serialize is using dataset_pillar which in turn uses pillar.get

init.sls:

{% from "h2o/map.jinja" import h2o with context -%}

h2o:
  pkg.installed:
    - name: {{ h2o.pkg }}
  service.running:
    - name: {{ h2o.service }}
  file.serialize:
    - name: /etc/h2o/{{ h2o.config_file }}
    - user: root
    - group: root
    - mode: 644
    - formatter: yaml
    - dataset_pillar: h2o:config
    - watch_in:
      - service: h2o

pillar: h2o.sls

h2o:
  lookup:
    pkg: h2o-git
  config:
    listen: 8080

    user: http

    access-log: /var/log/h2o-access.log

    hosts:
      "127.0.0.1.xip.io:8080":
        paths:
          /:
            file.dir: /srv/http
            file.dirlisting: 'ON'
            expires: 1 day

rendered config: /etc/h2o/h2o.conf

access-log: /var/log/h2o-access.log
hosts:
  127.0.0.1.xip.io:8080:
    paths:
      /:
        expires: 1 day
        file.dir: /srv/http
        file.dirlisting: 'ON'
listen: 8080
user: http

but it should be:/etc/h2o/h2o.conf

listen: 8080

user: http

access-log: /var/log/h2o-access.log

hosts:
  "127.0.0.1.xip.io:8080":
     paths:
       /:
         file.dir: /srv/http
         file.dirlisting: 'ON'
         expires: 1 day

@westurner
Copy link
Contributor Author

westurner commented Feb 19, 2015

seeAlso: CPython enhancement 16991 (Python 3.5) https://bugs.python.org/issue16991#msg232825 "Add OrderedDict written in C"

@uvsmtid
Copy link
Contributor

uvsmtid commented Nov 21, 2015

Should this issue be closed because issue #11599 is already closed?
See comment that OrderedDict is already used.

@basepi
Copy link
Contributor

basepi commented Nov 25, 2015

Good catch! This has indeed been implemented.

@basepi basepi closed this as completed Nov 25, 2015
@westurner
Copy link
Contributor Author

Thanks!
On Nov 24, 2015 6:10 PM, "Colton Myers" [email protected] wrote:

Closed #12161 #12161.


Reply to this email directly or view it on GitHub
#12161 (comment).

@genuss
Copy link
Contributor

genuss commented Oct 4, 2016

I'm terribly sorry to say that, but this issue isn't still fixed. #11599 was addressed to order in pillar top file, not pillar values.
There are several issues which come from this one: saltstack-formulas/nginx-formula#40 saltstack-formulas/nginx-formula#102 saltstack-formulas/nginx-formula#122 saltstack-formulas/php-formula#17

Can we move to using OrderedDicts in pillar and states?

@nicksloan
Copy link

I would love to see OrderedDicts everywhere as well. Ordered dicts are useful in a lot of cases where sequence matters, but you want to preserve the ability to override the value. I would think that this would be valuable for formula authors and end-users alike.

@mpibpc-mroose
Copy link

Me too. OrderedDicts make the behaviour mor predictive and intuitive.

@kungfoome
Copy link

Can this issue be re-opened?

@gabriel8fm
Copy link

Someone have any news about this issue? I have the same on our company salt environment and already tried to disable sort_keys on config.sls but doesn't work.

@rallytime rallytime reopened this Jun 8, 2018
@gabriel8fm
Copy link

gabriel8fm commented Jun 8, 2018

FYI We had problems when we tried to apply nginx state (with passenger) in Ubuntu 18.04 Bionic. The nginx.conf was created with the include /etc/nginx/modules-enabled/*.conf; at the end of file ordering alphabetically, causing nginx: [emerg] unknown directive error message. With this in mind we solve our problem puting the below block on nginx.conf template file:

{% if 'include' in config.keys() %}
{{ nginx_block(config.pop('include'), 'include') }}
{%- endif -%}

These setup can be used to another part of formula like pid, user, worker_processes or what you want.

Thanks for the moment.

Regards,

Gabriel Miranda

@stale
Copy link

stale bot commented Jan 9, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

@stale stale bot added the stale label Jan 9, 2020
@mitar
Copy link
Contributor

mitar commented Jan 9, 2020

Unstale.

@stale
Copy link

stale bot commented Jan 9, 2020

Thank you for updating this issue. It is no longer marked as stale.

@stale stale bot removed the stale label Jan 9, 2020
rhansen added a commit to rhansen/salt that referenced this issue Dec 19, 2022
Before, `!!omap` nodes were incorrectly assumed to be mapping nodes
when `dictclass` was not `dict`.  Now they are correctly processed as
sequences of single-entry mappings.  The resulting Python object has
type `dictclass` (usually `collections.OrderedDict` or a subclass).

This commit does not change the behavior when `dictclass` is `dict`;
loading an `!!omap` node still returns a list of (key, value)
tuples (PyYAML's default behavior).  I consider that to be a bug in
PyYAML, so a future commit may change the behavior in the `dict` case
to match the non-`dict` behavior.  (This commit uses `dictclass` for
the non-`dict` case to match what appears to be the original
intention.)

Behavior before:

```pycon
>>> import salt.utils.yaml as y
>>> import collections
>>> y.load("!!omap [{foo: bar}, {baz: bif}]")
[('foo', 'bar'), ('baz', 'bif')]
>>> y.SaltYamlSafeLoader("!!omap [{foo: bar}, {baz: bif}]", dictclass=collections.OrderedDict).get_single_data()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 51, in get_single_data
    return self.construct_document(node)
  File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 60, in construct_document
    for dummy in generator:
  File "salt/utils/yamlloader.py", line 42, in construct_yaml_omap
    return (yield from self.construct_yaml_map(node))
  File "salt/utils/yamlloader.py", line 36, in construct_yaml_map
    value = self.construct_mapping(node)
  File "salt/utils/yamlloader.py", line 52, in construct_mapping
    raise ConstructorError(
yaml.constructor.ConstructorError: expected a mapping node, but found sequence
  in "<unicode string>", line 1, column 1
```

Behavior after:

```pycon
>>> import salt.utils.yaml as y
>>> import collections
>>> y.load("!!omap [{foo: bar}, {baz: bif}]")
[('foo', 'bar'), ('baz', 'bif')]
>>> y.SaltYamlSafeLoader("!!omap [{foo: bar}, {baz: bif}]", dictclass=collections.OrderedDict).get_single_data()
OrderedDict([('foo', 'bar'), ('baz', 'bif')])
```

Relevant bug: saltstack#12161
rhansen added a commit to rhansen/salt that referenced this issue Dec 19, 2022
Now the behavior is consistent regardless of the value of the
Loader's `dictclass` constructor parameter.

Starting with Python 3.6, Python `dict` objects always iterate in
insertion order, so iteration order was already guaranteed when using
an ordinary YAML map.  However, `!!omap` provides a stronger guarantee
to users (plain map iteration order can be thought of as a Salt
implementation detail that might change in the future), and it allows
them to make their `.sls` files self-documenting.  For example,
instead of:

```yaml
my_pillar_data:
  key1: val1
  key2: val2
```

users can now do:

```yaml
my_pillar_data: !!omap
  - key1: val1
  - key2: val2
```

to make it clear to readers that the entries are intended to be
processed in order.

Behavior before:

```pycon
>>> import salt.utils.yaml as y
>>> import collections
>>> y.load("!!omap [{foo: bar}, {baz: bif}]")
[('foo', 'bar'), ('baz', 'bif')]
>>> y.SaltYamlSafeLoader("!!omap [{foo: bar}, {baz: bif}]", dictclass=collections.OrderedDict).get_single_data()
OrderedDict([('foo', 'bar'), ('baz', 'bif')])
```

Behavior after:

```pycon
>>> import salt.utils.yaml as y
>>> import collections
>>> y.load("!!omap [{foo: bar}, {baz: bif}]")
OrderedDict([('foo', 'bar'), ('baz', 'bif')])
>>> y.SaltYamlSafeLoader("!!omap [{foo: bar}, {baz: bif}]", dictclass=collections.OrderedDict).get_single_data()
OrderedDict([('foo', 'bar'), ('baz', 'bif')])
```

Relevant bug: saltstack#12161
@dmurphy18
Copy link
Contributor

@westurner Closing this due to age, the old version of Salt and Python 2.
Can you retest this with the latest release of Salt (currently 3006.3) and if still an issue, please open a new issue, which will have metrics in tracking issues.

Especially given the handling of Ordered Dictionaries in Python 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature new functionality including changes to functionality and code refactors, etc. order
Projects
None yet
Development

No branches or pull requests