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

Get the right target when using "__env__" on git ext_pillar to avoid merging problems #50417

Conversation

Projects
None yet
5 participants
@meaksh
Copy link
Member

commented Nov 7, 2018

What does this PR do?

This PR fixes an issue when multiple Git repositories are set as ext_pillar and they're using __env__ as the branch name.

When git_pillar_includes is enabled, the mechanism for extending and merging the different pillars is failing because the __env__ branch is not yet resolved:

pillar_roots.extend(
[d for (d, e) in six.iteritems(git_pillar.pillar_dirs)
if env == e and d != pillar_dir]

Because of that, given we have the following configuration:

  • On /etc/salt/master:
ext_pillar:
  - git:
    - __env__ git@gs.intern.priv:/srv/git/pillar:
      - name: gitinfo
      - privkey: "/etc/keys/id_rsa"
      - pubkey: "/etc/keys/id_rsa.pub"
    - __env__ git@gs.intern.priv:/srv/git/webapps:
      - name: webinfo
      - mountpoint: nowhere
      - privkey: "/etc/keys/id_rsa"
      - pubkey: "/etc/keys/id_rsa.pub"

First repository (gitinfo)

  • On the git server /srv/git/pillar/top.sls:
"{{saltenv}}":
  '*':
    - mytest 
    - nowhere.webinfo_external
  • On the git server /srv/git/pillar/mytest.sls:
mytest: added new stuff

Second repository (webinfo)

  • On the git server /srv/git/webapps/webinfo_external.sls:
mytest_external: True

Previous Behavior

# salt \* pillar.items

minion1.intern.priv:
    ----------
    _errors:
        - Specified SLS 'nowhere.webinfo_external' in environment 'master' is not available on the salt master
    mytest:
        added new stuff

New Behavior

# salt \* pillar.items

minion1.intern.priv:
    ----------
    mytest:
        added new stuff
    mytest_external:
        True

Tests written?

Yes

Commits signed with GPG?

Yes

meaksh added some commits Nov 7, 2018

@sathieu

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

I've been hit by this. Thanks for this PR.

@sathieu

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

can you add a test with one repo on top_only and the other on __env__?

@sathieu

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

Also, beware of the race condition (see #32245)

@cachedout cachedout requested a review from terminalmage Nov 8, 2018

@cachedout cachedout merged commit 73ce80f into saltstack:2018.3 Nov 8, 2018

9 of 10 checks passed

jenkins/pr/py2-windows-2016 The py2-windows-2016 job has failed
Details
WIP Ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has passed
Details

@meaksh meaksh deleted the meaksh:2018.3-fix-git_pillar-__env__-solving-for-merging branch Nov 9, 2018

@keesbos

This comment has been minimized.

Copy link
Contributor

commented on 59894e2 Jan 22, 2019

This breaks when repo.get_checkout_target() returns 'master' (or in fact repo.base)

Maybe:

env = repo.get_checkout_target()
if env == repo.base:
    env = 'base'

This comment has been minimized.

Copy link
Contributor

replied Jan 22, 2019

I'm not sure, but maybe it should be:

if repo.env:
    env = repo.env
else:
    env = repo.get_checkout_target()
if env == repo.base:
    env = 'base'
@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2019

Can you please be more specific than "this breaks"?

@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2019

I think I see actually, the target just returns the branch name, which is fine when the pillarenv is the branch, but it doesn't properly get identified as the base env when that branch is returned by get_checkout_target().

terminalmage added a commit to terminalmage/salt that referenced this pull request Jan 24, 2019

Fix regression in dynamic pillarenv
saltstack#50417 caused a regression in
which the env name is not properly detected as `base` when
`get_checkout_target()` returns the branch name corresponding to that
repo's `base` config option. This corrects that regression.

dincamihai added a commit to openSUSE/salt that referenced this pull request Feb 8, 2019

Fix regression in dynamic pillarenv
saltstack/salt#50417 caused a regression in
which the env name is not properly detected as `base` when
`get_checkout_target()` returns the branch name corresponding to that
repo's `base` config option. This corrects that regression.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.