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

KB* packages - Work around Boxstarter issue #1129

Merged
merged 1 commit into from
Oct 20, 2018

Conversation

catsburg
Copy link
Contributor

Description

Use module-qualified cmdlet name for Install-WindowsUpdate, to prevent accidental clash with the Install-WindowsUpdate command from BoxStarter.

Motivation and Context

When using Boxstarter to install chocolatey packages, the Install-WindowsUpdate command with the same name from Boxstarter gets called. This installs ALL critical Windows updates by default, which is unwanted behavior. To work around this issue, please ensure the correct Install-WindowsUpdate is called. See here the relevant boxstarter issue. The same fix was implemented by jberezanski on the KB-packages he maintains, see the discussion around this fix here and the commit with his fix here.

How Has this Been Tested?

This fix has been tested using Boxstarter on Windows Server 2016 Standard.

Before fix: Install-WindowsUpdate from BoxStarter.WinConfig is called, resulting in all critical Windows updates being installed.

After fix: Install-WindowsUpdate from chocolateyInstaller is called as expected.

See the relevant logging here.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Migrated package (a package has been migrated from another repository)

Checklist:

  • My code follows the code style of this repository.
  • My change requires a change to documentation (this usually means the notes in the description of a package). Does not require an update to the descriptions. I have however updated the release notes section for KB2999226 accordingly.
  • I have updated the documentation accordingly (this usually means the notes in the description of a package). Does not require an update to the descriptions. I have however updated the release notes section for KB2999226 accordingly.
  • All files are up to date with the latest Contributing Guidelines
  • The added/modified package passed install/uninstall in the chocolatey test environment. Can't get this working on my machine, maybe someone else can test this if it is deemed necessary for such a minor change?
  • The changes only affect a single package (not including meta package). As per section 3.4 in the contribution guidelines, this falls under the exception 'specific special cases that fix common problem'.

@AppVeyorBot
Copy link

✅ Package verification completed without issues. PR is now pending human review

@AdmiringWorm
Copy link
Member

I'll be honest, I'm a little hesitant about this change. I see the benefit, but I also see possible problems in the future. Let me explain.

We have no guarantee that the module name will be the same in the future, and if memory serves me right it never was the intention that all extension were to be under the chocolateyInstaller module name (I may be wrong though).

You may say that if the module name changes in the future, we could just change the module name being called but it isn't that simple.

Packages in this repository is expected to be compatible with the chocolatey versions that have been available between 1 year ago and today (unless otherwise specified, with for instance a dependency on chocolatey itself).

As such IMO it would be more beneficial to ask @jberezanski to perhaps update the extension package to use a different function name, and make an alias for Install-WindowsUpdate if no such function already exist.

That's just my two cents though.

@gep13
Copy link
Member

gep13 commented Oct 20, 2018

@AdmiringWorm I don't disagree with anything that you have said, however...

There is a very real problem just now with these KB packages getting installed via Boxstarter. Recently, the nodejs installer started offering this option in it's installer:

image

When you run this, it invokes Windows Update on the machine, installing all available updates. This isn't expected, as the only packages that are meant to be installed are these:

https://github.com/nodejs/node/blob/master/tools/msvs/install_tools/install_tools.txt

I would suggest that we merge this PR and get an update for these packages out, and then we can look to fix the issue properly. There is already a suggestion here:

chocolatey/boxstarter#343

To prefix the public functions with Boxstarter, so it is clearer what the intention is. I would suggest that this is the longer term issue that should be addressed.

Thoughts?

@AdmiringWorm
Copy link
Member

hmm, I'll say i'm still a bit hesitant.
But it's probably the best option we currently have, and as such I'll merge and get an update out for these packages.

@catsburg thanks for adding these changes.

@AdmiringWorm AdmiringWorm merged commit 5332b6b into chocolatey-community:master Oct 20, 2018
@catsburg
Copy link
Contributor Author

@AdmiringWorm Thanks for accepting the PR even though I agree this isn't ideal. We should now be able to install VS2017 and other software dependant upon KB-packages through Boxstarter again without triggering unwanted Windows Updates.

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.

4 participants