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

Modified setup.ps1 in order to show Windows Installation type #41002

Merged
merged 3 commits into from
Nov 15, 2019

Conversation

Gianlu
Copy link
Contributor

@Gianlu Gianlu commented Jun 1, 2018

SUMMARY

I added a line in setup.ps1 in order to present a new fact that states if the Windows installation is core or not. The variable is ansible_os_installation_type and its possible value is "Server Core" or "Server".

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

setup.ps1

ANSIBLE VERSION
ansible 2.5.3
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/usr/share/ansible/modules']
  ansible python module location = /usr/lib/python2.7/dist-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.13 (default, Nov 24 2017, 17:33:09) [GCC 6.3.0 20170516]
ADDITIONAL INFORMATION
$ ansible -m setup server1
...
"ansible_os_installation_type": "Server Core"
...

$ ansible -m setup server2
...
"ansible_os_installation_type": "Server"
...

@ansibot
Copy link
Contributor

ansibot commented Jun 1, 2018

@ansibot ansibot added affects_2.7 This issue/PR affects Ansible v2.7 community_review In order to be merged, this PR must follow the community review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. windows Windows community labels Jun 1, 2018
@dagwieers
Copy link
Contributor

I am unsure if this is the best way to do this.

What if the registry key is not found ? I'd expect an exception to be raised because InstallationType could not be found. So a safer way to do this would be advisable.

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Jun 1, 2018
@Gianlu
Copy link
Contributor Author

Gianlu commented Jun 1, 2018

Hi,
I postponed the switch -ea silentlycontinue so, if some exception is present, the object is null. The cast to string is useful to present an empty string to ansible output.
Moreover, I tested with a non existing registry key.

Let me know.

@dagwieers
Copy link
Contributor

So Powershell happily accepts $Null.InstallationType ?

@Gianlu
Copy link
Contributor Author

Gianlu commented Jun 1, 2018

Yes. It's quite strange but yes. But I submitted a new commit after your comment and I applied a check logic for the registry key.

Let me know if it is ok.

@dagwieers dagwieers requested review from nitzmahone and jborean93 June 1, 2018 12:03
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jun 11, 2018
Copy link
Contributor

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of querying the registry for this information. While there is little chance that the location may change it still isn't right. Unfortunately the alternative isn't that much nicer. The OperatingSystemSKU of Win32_OperatingSystem (which we already have access to) returns the int value of what we want but we still need to convert it to a string. Each int value corresponds to values in the table for pdwReturnedProductType in https://msdn.microsoft.com/en-us/library/ms724358(v=vs.85).aspx.

I would recommend you create a switch statement that gets the string from the int value for you like so;

$os_sku = switch($win32_os.OperatingSystemSKU) {
    0x00000000 { "undefined" }
    0x00000006 { "business" }
    0x00000010 { "business_n" }
    0x00000012 { "cluster_server" }
    .... # repeat ad-nauseam
    default { "unknown" }
}

When creating the string values we want to ensure they follow the Ansible standard and are in snake_case. I also don't think we need to include the PRODUCT_ prefix, e.g. PRODUCT_DATACENTER_SERVER_CORE would be datacenter_server_core

We gain a few benefits by going for this verbose approach

  • We are not reliant on checking if a registry exists and/or the user does not have enough rights to read the key
  • We are using a proper query to get the value, the onus is on MS to ensure this stays consistent in the future and not us
  • The return values are controlled by us, e.g. instead of Server Core we can format it in the standard Ansible way server_core (snake case)

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. small_patch and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jun 22, 2018
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jun 30, 2018
@dagwieers
Copy link
Contributor

Beware that the deadline for getting new features/modules accepted in Ansible v2.7 is nearing, it is set to either 2018-08-23 or 2018-08-30. If you are blocked, or you need feedback, please discuss on IRC channel #ansible-windows or add a comment to the Windows Working Group meeting agenda to get it resolved.

@ansibot ansibot added support:core This issue/PR relates to code supported by the Ansible Engineering Team. and removed support:community This issue/PR relates to code supported by the Ansible community. labels Sep 17, 2018
@ansibot
Copy link
Contributor

ansibot commented Feb 17, 2019

cc @if-meaton
click here for bot help

@ShachafGoldstein
Copy link
Contributor

according to this, in 2012 and above you're suppose to use th registry and only in 2008 and R2 use GetProductInfo

either way this is not simple to implement for 2008 - 2019+ unless using the registry @Gianlu started with

@jhawkesworth
Copy link
Contributor

Until there is an api call available in all versions of windows that ansible supports, this can't really be merged -setup.ps1 needs to be able to be run everywhere and I dislike the idea of not setting fact value under some circumstances.
Users with modern windows machines can work around by using their own custom facts module, so I am -1 on keeping this open.

@jborean93 jborean93 force-pushed the win_setup_install_type branch from 415b53e to 988a849 Compare November 15, 2019 01:05
@ansibot ansibot added support:community This issue/PR relates to code supported by the Ansible community. and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html small_patch stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Nov 15, 2019
@jborean93
Copy link
Contributor

I've added a changelog fragment and kept the original registry logic in here.

@jborean93 jborean93 merged commit bf8fe22 into ansible:devel Nov 15, 2019
anshulbehl pushed a commit to anshulbehl/ansible that referenced this pull request Dec 10, 2019
…e#41002)

* Modified setup.ps1 in order to show Windows Installation type

* Fix after pull request comment

* Added changelog fragment
@ansible ansible locked and limited conversation to collaborators Dec 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.7 This issue/PR affects Ansible v2.7 feature This issue/PR relates to a feature request. has_issue module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. windows Windows community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants