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

(MODULES-5383) Remove exit codes arg with feature #84

Merged
merged 2 commits into from Feb 13, 2019

Conversation

Projects
None yet
4 participants
@davidtwco
Copy link

davidtwco commented Aug 2, 2017

When the "usepackageexitcodes" feature is enabled with the
chocolateyfeature resource, this commit ensures that the
--ignore-package-exit-codes argument is not passed to the choco binary
and that the feature is respected.

Previously, the --ignore-package-exit-codes argument would be passed all
the time (on versions of Chocolatey where it existed) and this would
result in the enabling of the "usepackageexitcodes" feature to be
rendered moot.

return false if feature_tags.empty?
feature = feature_tags.first

return true if feature.attributes['enabled'].downcase == 'true'

This comment has been minimized.

@ferventcoder

ferventcoder Aug 17, 2017

This should check if it was explicitly enabled - <feature name="usePackageExitCodes" enabled="true" setExplicitly="false" description="[..snip..]" />

Otherwise this is likely to break all folks - the feature is turned on by default.

This comment has been minimized.

@davidtwco

davidtwco Aug 17, 2017

Author

I've changed this to check for the feature being enabled explicitly.

@ferventcoder
Copy link

ferventcoder left a comment

The biggest issue I see with this is that Puppet isn't able to handle non-zero values as successful (unless I am missing something). For the feature to work correctly, you would need to have a way to also tell Puppet that the exit codes indicate success.

@davidtwco

This comment has been minimized.

Copy link
Author

davidtwco commented Aug 17, 2017

@ferventcoder I agree. I haven't been using this feature for that reason (originally I thought it might help solve an issue I was having, which lead me to discover that the chocolateyfeature resource for this wasn't having an effect).

Despite this, I do think it makes sense to have the chocolateyfeature resource for this (as shown on the README) have an effect to avoid any confusion and support scenarios where this might be desired (whatever that may be).

@davidtwco

This comment has been minimized.

Copy link
Author

davidtwco commented Dec 4, 2017

@ferventcoder Have you had a chance to take a look at my last comment and this PR?

davidtwco added some commits Aug 2, 2017

(MODULES-5383) Remove exit codes arg with feature
When the "usepackageexitcodes" feature is enabled with the
chocolateyfeature resource, this commit ensures that the
--ignore-package-exit-codes argument is not passed to the choco binary
and that the feature is respected.

Previously, the --ignore-package-exit-codes argument would be passed all
the time (on versions of Chocolatey where it existed) and this would
result in the enabling of the "usepackageexitcodes" feature to be
rendered moot.
(MODULES-5383) Ensure feature was explicitly set.
Chocolatey enables the "usepackageexitcodes" by default, therefore this
will break existing behaviour of puppetlabs-chocolatey if it we do not
check that the feature was set explicitly.

This commit adds new methods to the provider to retrieve chocolatey
features from the configuration file. It uses these methods to
verify whether or not the `usepackageexitcodes` configuration setting
is explicitly set to enabled. If it is disabled or implicitly set,
the provider will add the `--ignore-package-exit-codes` parameter when
installing packages as is the existing behavior. If it is explicitly
set to enabled then the provider will *not* pass that parameter.

This is likely to break runs but respects the configuration settings.
@michaeltlombardi

This comment has been minimized.

Copy link

michaeltlombardi commented Feb 13, 2019

Hey, @davidtwco! Thank you so much for this PR! I've caught it up to current and added some additional tests, so we're going to merge! I super appreciate your work on this!

@jpogran jpogran merged commit 823d8fd into puppetlabs:master Feb 13, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.