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

Make test check_pillar more lenient #50022

Merged
merged 1 commit into from Oct 15, 2018

Conversation

Projects
None yet
3 participants
@bluesliverx
Copy link
Contributor

commented Oct 12, 2018

What does this PR do?

It makes the test.check_pillar state a bit more lenient when it comes to pillar loaded in using flat files. These values can be loaded as OrderedDict's and unicode strings, where currently the state checks for dict and str types only.

What issues does this PR fix or reference?

I did not create a corresponding issue since this was an extremely easy fix, but I can.

Previous Behavior

Using flat pillar files would load as OrderedDict's and unicode strings for me (not sure if this is just due to our current setup). These would fail simple test.check_pillar tests for dictionary and string types.

New Behavior

These values are seen as correct by test.check_pillar and pass with no issues.

Tests written?

Yes, and added a new test for the string types.

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@bluesliverx bluesliverx force-pushed the bluesliverx:lenient_test branch from edd47b9 to 4dd02b8 Oct 13, 2018

@bluesliverx bluesliverx requested a review from saltstack/team-core as a code owner Oct 13, 2018

@gtmanfred
Copy link
Contributor

left a comment

@bluesliverx can you rebase this so it only has your changes?

Thanks,
Daniel

@bluesliverx

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

Yup, sure can, sorry I submitted this at the end of the day and didn't take a look at it afterwards.

@bluesliverx bluesliverx force-pushed the bluesliverx:lenient_test branch 2 times, most recently from 506cc40 to 10abb13 Oct 15, 2018

@bluesliverx

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

@gtmanfred take a look now. I also fixed the pylint errors for py3.

@gtmanfred
Copy link
Contributor

left a comment

This seems fine for me.

@@ -334,7 +334,7 @@ def _check_key_type(key_str, key_type=None):
value = __salt__['pillar.get'](key_str, None)
if value is None:
return None
elif type(value) is not key_type and key_type is not None:
elif key_type is not None and type(value) is not key_type and not isinstance(value, key_type):

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Oct 15, 2018

Contributor

what is the point of adding the isinstance? that looks like what the type(value) is for?

This comment has been minimized.

Copy link
@bluesliverx

bluesliverx Oct 15, 2018

Author Contributor

In (at least) python 2, in certain situations the pillar data may be OrderedDicts instead of normal dicts, and the strings may be unicode instead of just strings. In this case, the dictionary check_pillar would fail since type({})!=OrderedDict, and the same for unicode strings.

I only noticed this problem when loading pillar data from flat files. Using extpillar providers (as we typically do) results in normal dict and str objects.

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Oct 15, 2018

Contributor

But they should always pass the isinstance check, which i would say would be good to remove the type(value) is not key_type, and just use the not isinstance.

This comment has been minimized.

Copy link
@bluesliverx

bluesliverx Oct 15, 2018

Author Contributor

Yes, I'm fine with that.

This comment has been minimized.

Copy link
@bluesliverx

bluesliverx Oct 15, 2018

Author Contributor

Updated with this change

@bluesliverx bluesliverx force-pushed the bluesliverx:lenient_test branch from 10abb13 to 3bb9663 Oct 15, 2018

@bluesliverx

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

@gtmanfred, since we are running into this on 2018.3, should I also make a PR against the 2018.3 branch? I'm not sure how you handle merging patches between versions.

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

No, but in the future, you should open the PR against the oldest supported branch that it applies to.

I have marked this to be back-ported to 2018.3 though.

@bluesliverx

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

Roger that, I'll do that in the future. Thanks!

@rallytime rallytime merged commit d1b3e95 into saltstack:develop Oct 15, 2018

11 checks passed

WIP ready for review
Details
codeclimate All good!
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/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

cachedout pushed a commit that referenced this pull request Oct 16, 2018

Mike Place
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.