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

Allow `/` in pillar includes #52008

Merged
merged 3 commits into from Apr 12, 2019

Conversation

@waynew
Copy link
Contributor

commented Mar 6, 2019

What does this PR do?

Re-allows / in pillar includes

What issues does this PR fix or reference?

#51832

Previous Behavior

Using fnmatch stopped us from allowing / in our pillar includes.

New Behavior

re-allow /

Tests written?

No(t yet)

Commits signed with GPG?

Yes


This works but I'm not certain if this works too broadly. For instance, if you wrote:

  include: foo/bar.bang/thing

It would look for foo.bar.bang.thing. Is that reasonable, or should I throw in a check to make sure that there is no . in the sub_sls before I do the replacement?

@waynew waynew requested a review from saltstack/team-core as a code owner Mar 6, 2019
Copy link
Contributor

left a comment

@waynew does this have a test that verifies the behavior?

@waynew

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

@dwoz not yet - I wanted to make sure that this behavior makes sense before I went and wrote the test for it.

i.e. should all of these be the same as include: foo.bar.bang.quux?

  • include: foo/bar/bang/quux
  • include: foo.bar/bang.quux
  • include: foo/bar.bang/quux

If the answer is yes, I'll get that test knocked out... otherwise... should I just make it so

  • include: foo/bar/bang/quux is equivalent, while include: foo.bar/bang.quux is something different?
@dmurphy18

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

@waynew also capturing it here, needs to support leading dot as in :

  • include: .foo.bar.bang.quux
@waynew waynew force-pushed the waynew:51832-re-allow-slash-includes branch from cb5dd42 to a7931ca Mar 11, 2019
@waynew

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

@dmurphy18 So... I have some mixed feelings about my current PR.

On the one hand, yes, it does work for allowing / and a leading .

The part I'm questioning is that technically this introduces a new feature, i.e. relative pillar includes. Before, a leading . worked, it was just a nop. Now the include will be relative to the path of the pillar file doing the include.

Should I break this up into multiple PRs, one restoring the behavior, and one with the improvement?

@dmurphy18

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

@waynew I would be in favor of splitting, an issue per fix is my motto.

@waynew waynew force-pushed the waynew:51832-re-allow-slash-includes branch 4 times, most recently from 3f63f00 to f76085e Mar 12, 2019
waynew added 3 commits Mar 6, 2019
This was raising an ignored exception because _closing was set later in
__init__. Not sure if it must be the last line in __init__.
@waynew waynew force-pushed the waynew:51832-re-allow-slash-includes branch from f76085e to ee3115f Apr 9, 2019
@dwoz dwoz added the 2019.2.1 label Apr 11, 2019
@dwoz dwoz merged commit b551bbd into saltstack:2019.2 Apr 12, 2019
10 checks passed
10 checks passed
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 Python lint test 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/py2-windows-2016 The py2-windows-2016 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
@dwoz
dwoz approved these changes Apr 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.