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

Fix pillar propagation in salt-ssh when overriding pillar in module.run + state.apply #47504

Merged
merged 2 commits into from May 16, 2018

Conversation

Projects
None yet
5 participants
@mateiw
Contributor

mateiw commented May 6, 2018

What does this PR do?

Add option to merge pillar with opts['pillar'] during pillar compilation. The options is False by default and only set by salt-ssh in order to allow for the pillar to be propagated when applying a state using module.run + state.apply and overriding the pillar explicitly.

This problem doesn't happen with regular, non-ssh minions.

What issues does this PR fix or reference?

#47501

Previous Behavior

If pillar is specified in module.run + state.apply it replaces any previous pillar data completely in the state that is applied instead of merging the value with the existing data.
E.g.

call_show:
  module.run:
    - name: state.apply
    - mods:
      - showpillar
    - kwargs: 
          pillar:    # <= replaces existing pillar data completely in the showpillar state
              test: "foo bar"

New Behavior

salt-ssh sets the option pillar_compile_merge_global to True in order to for the pillar compilation to merge the new data with the existing one.

Tests written?

No

Commits signed with GPG?

No

@mateiw mateiw requested a review from saltstack/team-core as a code owner May 6, 2018

@salt-jenkins salt-jenkins requested review from saltstack/team-ssh May 6, 2018

@cachedout cachedout requested a review from terminalmage May 7, 2018

@@ -911,6 +911,7 @@ def __init__(
'id': self.id,
'sock_dir': '/',
'fileserver_list_cache_time': 3,
'pillar_compile_merge_global': True

This comment has been minimized.

@isbm

isbm May 7, 2018

Contributor

This gets set to True all the time and ignores any kind of settings. I would be OK to set it to True by default, but completely ignoring the opposite value of it isn't the great idea. And maybe shorter name? like merge_pillar.

@mateiw mateiw force-pushed the mateiw:2018.3-salt-ssh-pillar-propagation-issue-47501 branch 2 times, most recently from 7fc332e to f2a0606 May 7, 2018

@mateiw

This comment has been minimized.

Contributor

mateiw commented May 7, 2018

Thanks for the suggestion @isbm. Renamed to pillar_merge_global and made it possible to override the default value using minion_opts in the roster file or globally using ssh_minion_opts in the master config.

@isbm

isbm suggested changes May 8, 2018 edited

@mateiw some more updates.

@@ -901,6 +901,7 @@ def __init__(
self.minion_opts = {
'grains_cache': True,
'log_file': 'salt-call.log',
'pillar_merge_global': True,

This comment has been minimized.

@isbm

isbm May 8, 2018

Contributor

I'd remove this and put it do the default ssh_..... options that comes from ssh_minion_opts. Besides, I would use conventional name ssh_<do>_<what>. That would be ssh_merge_pillar, and set it to True by default. Anyone who wants it void can set it to False for Salt SSH.

@mateiw mateiw force-pushed the mateiw:2018.3-salt-ssh-pillar-propagation-issue-47501 branch from f2a0606 to d46c60d May 8, 2018

@isbm

isbm approved these changes May 8, 2018

@mateiw thanks, perfect! Now we're good to go.

@terminalmage

This looks good, but we should document the new config option in doc/ref/configuration/minion.rst (with a .. versionadded:: 2018.3.2 to note when the option was added).

@mateiw mateiw force-pushed the mateiw:2018.3-salt-ssh-pillar-propagation-issue-47501 branch from d46c60d to 703de72 May 8, 2018

@mateiw

This comment has been minimized.

Contributor

mateiw commented May 8, 2018

@terminalmage I added the documentation for this new option.

@terminalmage

Thanks for adding the docs! Just one minor tweak needed here.

@@ -3219,3 +3219,31 @@ URL of the repository:
Replace ``<commit_id>`` with the SHA1 hash of a commit ID. Specifying a commit
ID is useful in that it allows one to revert back to a previous version in the
event that an error is introduced in the latest revision of the repo.
``ssh_merge_pillar``
----------------------

This comment has been minimized.

@terminalmage

terminalmage May 8, 2018

Member

Can you delete the last two dashes here? Sphinx will warn on the docs build when the dashed line is longer than the line above it.

@mateiw mateiw force-pushed the mateiw:2018.3-salt-ssh-pillar-propagation-issue-47501 branch from 703de72 to 2f1485e May 8, 2018

@mateiw

This comment has been minimized.

Contributor

mateiw commented May 8, 2018

@terminalmage removed dashes.

@mateiw mateiw referenced this pull request May 9, 2018

Merged

fixes for salt-ssh #17

@rallytime rallytime requested a review from terminalmage May 15, 2018

@rallytime rallytime merged commit 9d4f520 into saltstack:2018.3 May 16, 2018

4 of 9 checks passed

jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #4912 — ABORTED
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #9882 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #18968 — ABORTED
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #22845 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25095 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17199 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #21826 — SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment