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 flaky set_computer_name in mac_system module #54178

Merged
merged 4 commits into from Aug 12, 2019

Conversation

@dwoz
Copy link
Contributor

commented Aug 11, 2019

What does this PR do?

Use scutil instead of systemsetup to set and get the computer name on MacOS. systemsetup is from 2002 and is not reliable at setting the computer name.

(venv) root@carbon:~/src/salt-forge/2019.2.1-tests/salt# systemsetup -setcomputername "RS-KFQZY1"
setcomputername: RS-KFQZY1
(venv) root@carbon:~/src/salt-forge/2019.2.1-tests/salt# systemsetup -getcomputername
Computer Name: RS-KFQZYP
(venv) root@carbon:~/src/salt-forge/2019.2.1-tests/salt# systemsetup -getcomputername
Computer Name: RS-KFQZYP
(venv) root@carbon:~/src/salt-forge/2019.2.1-tests/salt# systemsetup -getcomputername
Computer Name: RS-KFQZYP
(venv) root@carbon:~/src/salt-forge/2019.2.1-tests/salt# systemsetup -getcomputername
Computer Name: RS-KFQZYP
(venv) root@carbon:~/src/salt-forge/2019.2.1-tests/salt# systemsetup -getcomputername
Computer Name: RS-KFQZYP
(venv) root@carbon:~/src/salt-forge/2019.2.1-tests/salt# systemsetup -setcomputername "RS-KFQZY1"
setcomputername: RS-KFQZY1
(venv) root@carbon:~/src/salt-forge/2019.2.1-tests/salt# systemsetup -getcomputername
Computer Name: RS-KFQZYP
(venv) root@carbon:~/src/salt-forge/2019.2.1-tests/salt# systemsetup -getcomputername
Computer Name: RS-KFQZYP
(venv) root@carbon:~/src/salt-forge/2019.2.1-tests/salt# systemsetup -getcomputername
Computer Name: RS-KFQZYP

scutil is from 2008 and scutil --set ComputerName "RS-KFQZY1" works reliably.

Tests written?

No - Has existing test coverage that was failing

Commits signed with GPG?

Yes

dwoz added 2 commits Aug 11, 2019
@dwoz dwoz force-pushed the dwoz:mac_computer_name branch from fc4b375 to a3a9d92 Aug 11, 2019
@Ch3LL
Ch3LL approved these changes Aug 12, 2019
Copy link
Contributor

left a comment

Great find! I think there are other modules that use systemsetup as well that are flaky. That command is just flaky in general so might be good to switch to a different command if available in those modules too if they keep failing in the future. The other modules are mac_timezone.py and mac_power.py

@dwoz

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

Great find! I think there are other modules that use systemsetup as well that are flaky. That command is just flaky in general so might be good to switch to a different command if available in those modules too if they keep failing in the future. The other modules are mac_timezone.py and mac_power.py

I agree.

@Akm0d
Akm0d approved these changes Aug 12, 2019
@dwoz dwoz merged commit 19f4d5e into saltstack:2019.2.1 Aug 12, 2019
23 of 24 checks passed
23 of 24 checks passed
ci/py2/centos7 This commit is being built
Details
WIP Ready for review
Details
ci/docs This commit looks good
Details
ci/lint This commit looks good
Details
ci/py2/amazon2 This commit looks good
Details
ci/py2/centos6 This commit looks good
Details
ci/py2/centos7/tcp This commit looks good
Details
ci/py2/debian8 This commit looks good
Details
ci/py2/debian9 This commit looks good
Details
ci/py2/fedora29 This commit looks good
Details
ci/py2/ubuntu1604 This commit looks good
Details
ci/py2/ubuntu1604/tcp This commit looks good
Details
ci/py2/ubuntu1804 This commit looks good
Details
ci/py2/windows2016 This commit looks good
Details
ci/py3/amazon2 This commit looks good
Details
ci/py3/centos7 This commit looks good
Details
ci/py3/centos7/tcp This commit looks good
Details
ci/py3/debian8 This commit looks good
Details
ci/py3/debian9 This commit looks good
Details
ci/py3/fedora29 This commit looks good
Details
ci/py3/ubuntu1604 This commit looks good
Details
ci/py3/ubuntu1604/tcp This commit looks good
Details
ci/py3/ubuntu1804 This commit looks good
Details
ci/py3/windows2016 This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.