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

Add operating system and edition checks (fixes #18) #78

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

thejsa
Copy link

@thejsa thejsa commented Oct 26, 2018

Adds checks for operating system version, SKU, and architecture (where applicable) to WSL, Docker, Hyper-V, app removal, and developer mode enable scripts.

@msftclas
Copy link

msftclas commented Oct 26, 2018

CLA assistant check
All CLA requirements met.

@yodurr yodurr requested a review from gep13 November 15, 2018 06:34
@yodurr
Copy link
Contributor

yodurr commented Nov 15, 2018

Hi @thejsa this is pretty cool to see. Thank you. I need to loop in others from Chocolatey as I'm not familiar with the way you're version checking. This does seem like a valuable addition.

Can we centralize the versioning checking logic into helper functions so the scripts can just say if (Is64Bit()) etc?

@thejsa
Copy link
Author

thejsa commented Nov 16, 2018

Can we centralize the versioning checking logic into helper functions so the scripts can just say if (Is64Bit()) etc?

Certainly; that seems far more elegant; I'll work on that.

@thejsa
Copy link
Author

thejsa commented Nov 16, 2018

Complete in commit f3bad0f - haven't been able to spin up a VM to test this commit yet (will be able to once my main box is back up) but testing the scripts individually seems to yield no problems.

@AdmiringWorm
Copy link

Just throwing this out here, so people are aware of it:

chocolatey also have a helper to check compare for 64/32 bit called Get-OSArchitectureWidth (The following aliases is also available: Get-OSBitness, Get-ProcessorBits)

That helper can be used in two ways:
Comparison:

if (Get-OSArchitectureWdith -Compare 64) {
  # Do whatever work is needed to be done for 64bit OS's
}

Or get the actual bit

$bits = Get-OSArchitectureWidth # Will output either 32 or 64 depending on the architecture.

@yodurr
Copy link
Contributor

yodurr commented Nov 19, 2018

Thanks for doing this @thejsa I'll test this out this week.

@yodurr yodurr requested a review from ferventcoder November 19, 2018 18:14
@@ -1,30 +1,39 @@
choco install -y Microsoft-Windows-Subsystem-Linux --source="'windowsfeatures'"
if (
Copy link
Contributor

@yodurr yodurr Nov 21, 2018

Choose a reason for hiding this comment

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

A couple suggestions

  1. you can install WSL on some versions of Server: https://docs.microsoft.com/en-us/windows/wsl/install-on-server
  2. With this logic block I think it will be better to put it into a Is-WSL-Capable function in CompabitibilityChecks and use that here. Also a if !Is-WSL-Capable and return is cleaner than putting the rest of the file inside the then { }

Copy link
Author

Choose a reason for hiding this comment

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

  1. Yep, this permits FCU and newer Windows Server builds to install WSL
  2. That certainly seems cleaner; could also allow use elsewhere for possible future dependencies on WSL. I'll do that

Copy link

@bcurran3 bcurran3 Nov 22, 2018

Choose a reason for hiding this comment

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

(I just looked at the wsl install script, I hope I'm commenting in the right place...trying to be helpful.)

There are some gotchas on the installing the various distros, not all have a simple install options that then return you to the command prompt, thus breaking automation without some extra effort.

You can look at my Chocolatey package install scripts for examples.
https://chocolatey.org/packages/wsl-ubuntu-1804
https://chocolatey.org/packages/wsl-ubuntu-1604
https://chocolatey.org/packages/wsl-debiangnulinux
https://chocolatey.org/packages/wsl-kalilinux
https://chocolatey.org/packages/wsl-opensuse
https://chocolatey.org/packages/wsl-archlinux
https://chocolatey.org/packages/wsl-sles

Maybe distros would be better handled by choco install commands? (Yes, I'm open to improvements in my install scripts.)

Choose a reason for hiding this comment

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

(Cross posted from #23 (comment))

I mention this because what I saw in the commit 969035c for SLES and openSUSE won't work based on my experience.

openSUSE REF: https://github.com/bcurran3/ChocolateyPackages/blob/master/wsl-opensuse/tools/ChocolateyInstall.ps1
SLES REF: https://github.com/bcurran3/ChocolateyPackages/blob/master/wsl-sles/tools/ChocolateyInstall.ps1

Choose a reason for hiding this comment

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

Also note that WSL requires a reboot before use/installing distros.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @bcurran3. I agree the choco install commands make it easier to install the distro. Thank you for taking initiative in making these. I'd like the distro owners to review your choco packages before we add them to this project. I'll discuss with @tara-raj and we can start going through the list of distro owners.

A challenge with installing a WSL distro as part of a boxstarter script is the default user creation so we can also automate the install of tools inside the distro. In the case of Ubuntu we use the default root user with empty password, allowing us to then automate install of packages. After the boxstarter script completes the user is encouraged to then create a new user in Ubuntu with a non-blank password. I see you do the same thing. As long as the user remembers to go in and create a user with a non-blank password and use that as the default then they're back in the same setup as when installing from the Store. It may be possible to improve on this so a choco install and Store install produce the same first run experience - this is a topic I'd love to hear feedback on from the community and distro owners.

Copy link
Contributor

Choose a reason for hiding this comment

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

moving the above comment to issue #32

Choose a reason for hiding this comment

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

@yodurr I'm game for improvements. My scripts for the installs are not the best, but the best I could do. I welcome feedback/changes/PRs. :)

@@ -21,6 +21,8 @@ function executeScript {
iex ((new-object net.webclient).DownloadString("$helperUri/$script"))
}

executeScript "CompatibilityChecks.ps1";
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think of putting these include statements inside the helper scripts that use them instead of in the recipes? Since the helper scripts now have a dependency on CompatibilityChecks it would be best to add the include there or the helper scripts become more fragile.

Copy link
Author

Choose a reason for hiding this comment

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

Hadn't thought about that (fairly new to PowerShell scripting, more used to Python) but that does make sense

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.

5 participants