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 possible issue with test_groupadd #49794

Merged
merged 2 commits into from Oct 17, 2018

Conversation

Projects
None yet
5 participants
@twangboy
Copy link
Contributor

commented Sep 26, 2018

What does this PR do?

In Windows, the group.members function returns a dicts where the result key returns true if successful. On Linux it just returns True or False. The way the test was written the check would always be True even on failure because an empty Dict results in True if you're just checking with assertTrue.

What issues does this PR fix or reference?

This checks the value in result on Windows

Tests written?

Yes

Commits signed with GPG?

Yes

twangboy added some commits Sep 26, 2018

@twangboy twangboy requested review from saltstack/team-core Sep 26, 2018

@dwoz

dwoz approved these changes Sep 26, 2018

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Sep 30, 2018

Why did these two platforms diverge in the first place? Would it make sense to just bring them in line with each other?

@damon-atkins

This comment has been minimized.

Copy link
Member

commented Sep 30, 2018

@cachedout agree they should be the same. The state expects a true or false from all platforms as its called in a middle of an if.

Normal state outputs comments, changes, general modules do not.

@damon-atkins

This comment has been minimized.

Copy link
Member

commented Sep 30, 2018

if __salt__['group.add'](name, gid=gid, system=system):

However the above seems wrong. The group.add or group.* on all platforms should raise a CommandExecutionError('some error message') and the group state should trap it.

What should the standard be? i.e. more like pkg state? Windows group.add is sort of following.

@twangboy

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2018

Now returns bool instead of dict

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2018

@twangboy and @dwoz Is that change in structure something we want to do in a dot release? I think that might be better for a feature release. Thoughts?

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Oct 16, 2018

I agree with @rallytime . I think we should consider a change in the structure in a major release but not change it here.

@twangboy twangboy force-pushed the twangboy:fix_groupadd_test branch from b43439a to 3d3b67d Oct 16, 2018

@twangboy

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

I reverted back to just the original test fix I did. I created another PR against the Fluorine branch with the fixes I had before. #50086

@rallytime rallytime merged commit 7ec3840 into saltstack:2017.7 Oct 17, 2018

7 of 10 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has failed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has failed
Details
WIP ready for review
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

@twangboy twangboy deleted the twangboy:fix_groupadd_test branch Dec 27, 2018

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.