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 windows osrelease grain #36379

Merged
merged 12 commits into from Sep 20, 2016
Merged

Conversation

twangboy
Copy link
Contributor

What does this PR do?

Properly gets the osrelease grain for Server versions of Windows.

What issues does this PR fix or reference?

#36307

Previous Behavior

OS name was removed in the python command platform.uname(). Python version 2.7.8 would return a value of "2008ServerR2" for Windows Server 2008 R2. We moved to Python 2.7.11 and later to 2.7.12 which both report the Desktop version of 7 instead of the server name of 2008ServerR2.

New Behavior

Properly detect windows server os release grain along with any applied service packs.

Tests written?

No

@cachedout
Copy link
Contributor

Hi @twangboy

The code that was copied from StackOverflow needs to be linted so that it passes tests, please.

As far as the overall change goes, are you certain that you want to change core grains in a point release? Traditionally, this has been something that we have not done. This may make more sense in a major release.

@lorengordon
Copy link
Contributor

@cachedout, this grain has already changed in a point release, as a result of updating the Python version distributed with Windows. For example, what used to return 2008ServerR2 now returns 7.

However, I would personally prefer to maintain the grain value that was returned previously... e.g. 2008ServerR2 vs Server 2008 R2. I'm not sure why that would be changed, point release or otherwise. Using the same logic, neither should the Service Pack info be added to this grain.

@UtahDave
Copy link
Contributor

I agree with @lorengordon, we should restore the grain to what it was before 2008ServerR2, etc.
We'll move the Service Pack to a new grain.

@damon-atkins
Copy link
Contributor

How does PY3 behavior?

@twangboy
Copy link
Contributor Author

Py3 returns the same as Py2.7.12

@lorengordon
Copy link
Contributor

PY3 also depends on the python version:

PS C:\Users\Administrator> C:\Python33\python.exe --version
Python 3.3.5
PS C:\Users\Administrator> C:\Python33\python.exe -c "import platform;print(platform.win32_ver())"
('2012Server', '6.2.9200', '', 'Multiprocessor Free')
PS C:\Users\Administrator> C:\Python34\python.exe --version
Python 3.4.4
PS C:\Users\Administrator> C:\Python34\python.exe -c "import platform;print(platform.win32_ver())"
('2012ServerR2', '6.3.9600', '', 'Multiprocessor Free')
PS C:\Users\Administrator> C:\Python35\python.exe --version
Python 3.5.2
PS C:\Users\Administrator> C:\Python35\python.exe -c "import platform;print(platform.win32_ver())"
('8.1', '6.3.9600', 'SP0', 'Multiprocessor Free')

@cachedout
Copy link
Contributor

How are we all feeling about the latest changes here?

@lorengordon
Copy link
Contributor

Works for me if it works for you. I tested the patch and it does fix the issue.

PS C:\Users\Administrator> C:\salt\salt-call.bat --version
salt-call 2015.8.12 (Beryllium)
# Without the patch
PS C:\Users\Administrator> C:\salt\salt-call.bat --local grains.get osrelease
local:
    8.1
# With the patch
PS C:\Users\Administrator> C:\salt\salt-call.bat --local grains.get osrelease
local:
    2012ServerR2
PS C:\Users\Administrator>

@cachedout cachedout merged commit 266dd7c into saltstack:2015.8 Sep 20, 2016
@cachedout
Copy link
Contributor

Thanks, @lorengordon. I'll go ahead and get this in then.

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

5 participants