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

Add defaults.deepcopy function. #44851

Merged
merged 2 commits into from Dec 7, 2017
Merged

Add defaults.deepcopy function. #44851

merged 2 commits into from Dec 7, 2017

Conversation

aabouzaid
Copy link
Contributor

@aabouzaid aabouzaid commented Dec 6, 2017

What does this PR do?

Expose copy.deepcopy as defaults.deepcopy function.

What issues does this PR fix or reference?

In many case there are multi level of defaults, e.g. global, group, and specific.
And since Python does not copy objects by default but creates bindings, so deep copy is necessary for that cases.

Here is an arbitrary example:

Yaml

vars:
  hosts:
    defaults:
      enabled: True
      aggregate: source
      extra:
        - test
        - stage
    list:
      hos01:
        index: foo
        upstream: bar
      hos02:
        index: foo2
        upstream: bar2

Jinja

{% for host, vars in vars.hosts.list.items() %}
  {% set defaults = salt['defaults.deepcopy'](vars.hosts.defaults) %}
  {% set host_vars = salt['defaults.merge'](defaults, vars) %}
  ....
{% endfor %}

Output

hos01:
  index: foo
  upstream: bar
  enabled: True
  aggregate: source
  extra:
    - test
    - stage

New Behavior

Allow to make deepcopy of objects.

Tests written?

Yes

Commits signed with GPG?

Not yet.

@noelmcloughlin
Copy link
Contributor

Nice idea. This jinja2 example is more efficient?

{% set defaults = salt['defaults.deepcopy'](vars.hosts.defaults) %}
{% for host, vars in vars.hosts.list.items() %}
 {% set host_vars = salt['defaults.merge'](defaults, vars) %}
 ....
{% endfor %}

Copy link
Contributor

@noelmcloughlin noelmcloughlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks useful for jinja scenarios. thanks!!

@aabouzaid
Copy link
Contributor Author

@noelmcloughlin

Nope, that's actually the current behavior with normal copy because defaults.merge is merging in place and it doesn't return.

The defaults will be overwritten within 1st iteration.

Here is an example:

vars:
  hosts:
    defaults:
      enabled: True
    list:
      hos01:
        enabled: False
        index: foo
      hos02:
        index: foo2
{% set defaults = salt['defaults.deepcopy'](vars.hosts.defaults) %}
{% for host, vars in vars.hosts.list.items() %}
 {% set host_vars = salt['defaults.merge'](defaults, vars) %}
 ....
{% endfor %}
before loop:
defaults = { enabled: True }

itr01 (host01):
defaults = { enabled: False, index = foo }

itr02 (host02):
defaults = { enabled: False, index = foo2 }

That's why it should make a real copy inside the loop.

@noelmcloughlin
Copy link
Contributor

Thanks for the clear explanation!! @aabouzaid

Copy link
Contributor

@rallytime rallytime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tests, too. 👍

@rallytime
Copy link
Contributor

@aabouzaid
Copy link
Contributor Author

@rallytime a typo 🙆‍♂️
Fixed :-)

@rallytime rallytime merged commit b6173d6 into saltstack:develop Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants