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

Populate changes dictionary on system.join_domain #49739

Merged
merged 3 commits into from Oct 4, 2018

Conversation

Projects
None yet
3 participants
@twangboy
Copy link
Contributor

commented Sep 21, 2018

What does this PR do?

Fixes issue with the system.join_domain state where the changes dictionary was not being populated when changes were made to the system.

  • Populate the changes dictionary
  • Fix docs formatting
  • Clean up code
  • Use explicit function calls
  • Add comment about reboot status

What issues does this PR fix or reference?

#49660

Tests written?

No

Commits signed with GPG?

Yes

Populate changes dictionary
Fix docs formatting
Clean up code
Use explicit function calls
Add comment about reboot status

@salt-jenkins salt-jenkins requested a review from saltstack/team-windows Sep 21, 2018

@ghost

This comment has been minimized.

Copy link

commented Sep 21, 2018

Thanks @twangboy. I have to point out that I think that the restart switch for this function should probably be deprecated, since we now have the system.reboot state (which is supposed to know about the pending domain join). Also the restart switch comes with no options, which means that it always has a five minute timeout. A sysadmin doing a domain join typically won't want a five minute timeout at that time since joining is usually a deployment step. Even if the reboot flag isn't deprecated it should result in an immediate reboot.

@dwoz

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

@twangboy Can you look into this test failure?

integration.modules.test_groupadd.GroupModuleTest.test_members
https://jenkinsci.saltstack.com/job/pr-windows2016-py2/job/PR-49739/1/

@twangboy

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

@dwoz The above failure is not related to this PR. However, I will look at the test to see if maybe it's flaky.

@twangboy

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

@dwoz I can't replicate it locally, but it's doing several run_functions. Maybe one of them is failing. I modified the test so that it actually checks the results of the group.members function on Windows.

#49794

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

@dwoz since @twangboy is addressing that test concern in another PR, how does this look to you now?

@dwoz

dwoz approved these changes Oct 1, 2018

@dwoz

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2018

@rallytime That seems okay to me.

@rallytime rallytime merged commit b959033 into saltstack:2017.7 Oct 4, 2018

7 of 10 checks passed

jenkins/pr/py2-windows-2016 The py2-windows-2016 job has failed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has failed
Details
jenkins/pr/py3-windows-2016 The py3-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-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details

@twangboy twangboy deleted the twangboy:fix_49660 branch Oct 4, 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.