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

Fix #22063: pillar wildcard support include #45269

Merged
merged 19 commits into from
Jan 26, 2018

Conversation

wych42
Copy link
Contributor

@wych42 wych42 commented Jan 4, 2018

What does this PR do?

Wildcard in pillar support include. e.g.:

include:
  - sub_include_state_*

will render every state file matches pattern sub_include_state_*.

What issues does this PR fix or reference?

#22063

Tests written?

Not yet, working on it.

Commits signed with GPG?

Yes/partial.

Copy link
Contributor

@terminalmage terminalmage left a comment

Choose a reason for hiding this comment

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

Looks good, pending tests being added.

@wych42
Copy link
Contributor Author

wych42 commented Jan 6, 2018

@terminalmage I have add a test on this, but the jenkins pipeline didn't pass, I can't reproduce the failure tests on my local machine either Mac or Ubuntu host. Now I have no clue how to debug this, can I get some help from you?

@wych42
Copy link
Contributor Author

wych42 commented Jan 10, 2018

CI failed because

06:52:42  * Starting salt-syndic ... 06:52:41,529 [salt.modules.vsphere                     :213 ][ERROR   ] pyVmomi not loaded: Incompatible versions of Python. See Issue #29537.

It looks like python version in jenkins is

06:43:55 * Python Version: 2.7.5 (default, Aug 4 2017, 00:39:18) [GCC 4.8.5 20150623 (Red Hat 4.8.5-16)]

which is not compatible with pyVmomi.

@rallytime
Copy link
Contributor

re-run py

1 similar comment
@rallytime
Copy link
Contributor

re-run py

@cachedout
Copy link
Contributor

cachedout commented Jan 11, 2018

@rallytime It has been too long since I have been in this code for me to knowledgeably review it. I think we should have some more core folks go over it.

@cachedout cachedout removed their request for review January 11, 2018 23:52
@rallytime
Copy link
Contributor

@cachedout Of course, no problem!

Copy link
Contributor

@terminalmage terminalmage left a comment

Choose a reason for hiding this comment

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

@marlowew I don't understand why you added your tests inside a test function for malformed pillar SLS. Don't they belong in their own test?

Also, the pyVmomi error is completely unrelated and it is not causing any failures. It's also not referenced at all in tests/unit/test_pillar.py. That error is nothing to worry about, if we can't import that module then we just end up skipping the tests that use it.

@wych42
Copy link
Contributor Author

wych42 commented Jan 15, 2018

@terminalmage You are right. I have pushed another commit and put unit test for include pillar in separated function.

@garethgreenaway Please have another look at the latest commit.

Copy link
Contributor

@terminalmage terminalmage left a comment

Choose a reason for hiding this comment

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

@wych42
Copy link
Contributor Author

wych42 commented Jan 22, 2018

@terminalmage The last commit has solved broken malformed_pillar_sls test.

@rallytime
Copy link
Contributor

@wych42 wych42 requested a review from a team as a code owner January 26, 2018 05:18
@wych42
Copy link
Contributor Author

wych42 commented Jan 26, 2018

@rallytime My bad, and it's fixed.

@rallytime
Copy link
Contributor

It's all good. Thanks @marlowew!

@rallytime rallytime merged commit b13b897 into saltstack:develop Jan 26, 2018
@wych42 wych42 deleted the pillar_wildcard_include branch January 26, 2018 15:37
@cachedout
Copy link
Contributor

@rallytime Will this make it into Oxygen?

@rallytime
Copy link
Contributor

rallytime commented Feb 7, 2018

@cachedout No, it was submitted against the develop branch. It will be in Fluorine.

@anitakrueger
Copy link
Contributor

Any chance you could port this into the 2018.3 branch? I think a lot of people will be benefit from this and it was in the code base for 2 months before Oxygen was released.

@gtmanfred
Copy link
Contributor

@anitakrueger unfortunately it was submitted after the feature freeze for the 2018.3 release, which we need in order to stabilize 2018.3, so it will have to wait until the next feature release, which will be Fluorine.

@anitakrueger
Copy link
Contributor

@gtmanfred thanks for letting me know.

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

8 participants