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

Control enhanced exit codes with a feature switch #1758

Closed
ferventcoder opened this issue Mar 14, 2019 · 11 comments
Closed

Control enhanced exit codes with a feature switch #1758

ferventcoder opened this issue Mar 14, 2019 · 11 comments

Comments

@ferventcoder
Copy link
Member

ferventcoder commented Mar 14, 2019

By default useEnhancedExitCodes feature will be turned on. Over time we'll be adding enhanced exit codes to more and more commands. This is related to #512, but not tied to the package itself providing an exit code.

This feature switch is being provided as a compatibility for integrations (like config managers) to continue to working if there is an issue with seeing new codes prior to having a new release of those integrations.

Release Notes

We've listed the issues related to this (#1602 #1724, especially #1724) as possibles breaking changes as it may affect tools that are integrating with Chocolatey and interpreting the output of the exit code. In these cases, it would likely temporarily break those tools until they've had a chance to release new versions of their tools. If you run into this, you simply need to turn off the feature "useEnhancedExitCodes". That is as simple as choco feature disable --name="'useEnhancedExitCodes'"

@ferventcoder ferventcoder added this to the 0.10.12 milestone Mar 14, 2019
@ferventcoder ferventcoder self-assigned this Mar 14, 2019
@ferventcoder ferventcoder changed the title Enhanced Exit Codes Feature feature - Enhanced Exit Codes to control if exit codes are better than simply 0 or 1 Mar 14, 2019
@ferventcoder ferventcoder changed the title feature - Enhanced Exit Codes to control if exit codes are better than simply 0 or 1 feature - useEnhancedExitCodes to control if exit codes are better than simply 0 or 1 Mar 14, 2019
@ferventcoder ferventcoder changed the title feature - useEnhancedExitCodes to control if exit codes are better than simply 0 or 1 feature - useEnhancedExitCodes to control if exit codes are better than simply 0 or 1 (default: ON) Mar 14, 2019
@ferventcoder ferventcoder changed the title feature - useEnhancedExitCodes to control if exit codes are better than simply 0 or 1 (default: ON) Control enhanced exit codes with a feature switch Mar 14, 2019
ferventcoder added a commit that referenced this issue Mar 14, 2019
In #1602 and #1724, enhanced exit codes were added to provide more
intentional exit codes that determine the state of what happened during
the run. This would allow simply seeing a 0 from outdated and knowing
that all packages are up to date, or seeing a 2 and knowing that
packages need updated. This allows for better scripting based on exit
codes.

However, there are some existing integrations that might be broken on
taking on a newer version of Chocolatey if the enhanced exit codes are
not being looked for yet. To allow compatibility to older systems, add
a feature switch that can be disabled to provide the older behavior of
0 or 1 exit codes.
ferventcoder added a commit that referenced this issue Mar 14, 2019
Add exit code documentation to all of the commands.
ferventcoder added a commit that referenced this issue Mar 15, 2019
* stable:
  (doc) add exit codes to gen doc headings
  (doc) update generated docs
  (version) 0.10.12
  (doc) fix grammar
  (GH-1038) remove note on off recommendation
  (GH-1746) allow shutting off validation warnings
  (doc) update release notes for licensed extension
  (maint) whitespace
  (doc) update release notes
  (maint) formatting
  (GH-1758)(doc) provide exit code docs
  (doc) info - add usage / examples
  (doc) outdated - note ignore unfound
  (doc) scripting - point to scripting reference
  (doc) scripting - how to get exit code w/powershell
  (GH-1724) search - exit 2 on no results
  (GH-1758) control enhanced exit codes w/feature
  (doc) update release notes for 0.10.12 release
  (doc) update older release notes
@ferventcoder
Copy link
Member Author

ferventcoder commented Mar 15, 2019

Here is the ansible fix - ansible/ansible#53841 (thank you @jborean93 for catching that so quickly - before we sent our announce email noting the potential breaking change)
For other config managers:

@jborean93
Copy link

Heh no worries, our CI was failing within an hour of the release so was pretty easy to pick up. I’ll let people know about the feature to disable the enhanced codes but a new release of Ansible with the fix should be out relatively soon.

@jborean93
Copy link

@ferventcoder I didn't want to comment on each of the new issues you've raised but I really think you should reconsider making this the default and the enhanced exit codes should be opt in instead of opt out. As you've seen there have been multiple issues, I've closed 3 in ansible/ansible this past 24 hours due to this effectively breaking the module. Typically an exit code that is non 0 means an error and because historically this was the case for many choco.exe commands I think it was wrong to change it, especially with so little notice.

The other issue is that by default the win_chocolatey module and the PS script on Chocolatey's website installs the latest making it harder for people to just say I want the older version until they've upgraded Ansible to one that supports v0.10.12+. I'm open to suggestions here, I couldn't find anything after doing a very basic search.

If you are still wanting to push ahead with this change on other commands like feature, source, etc, I'm going to have to make changes to 3 more Chocolatey modules we have to ignore an rc of 2 and hopefully get that out in an Ansible release before 0.10.14 is out. This is all well and good for people that are upgrading Ansible on each regular release but I can guarantee that there are numerous people who cannot and will not upgrade for a long time. This means that the Chocolatey modules in Ansible are effectively broken or require a new and relatively obscure feature to be set which is not very intuitive.

I can't talk about the other config management software but this definitely hit us out of the blue and was surprising that a breaking change was made. It makes us worried what other breaking changes will be made in future Chocolatey versions that effectively break the modules we have. Even having a release candidate of a newer version that we can run our integration tests against would be great.

@nitzmahone
Copy link

nitzmahone commented Mar 18, 2019

... or at least delay the switch to the default until after projects that rely on choco have had a chance to accommodate it. As-is, a lot of working automation in the world will be is being broken by this change until the various projects have had a chance to release fixes, and users have upgraded.

@charlieok
Copy link

charlieok commented Mar 18, 2019

I was also just hit by this. I would add that for a change like this, which you even described in the release notes as "possible breaking changes", you should bump a more significant component of the version number. For instance 0.10.11 -> 0.11.1 rather than 0.10.11 -> 0.10.12.

That would have signaled a more significant update, which may require attention, and software configured with the intent of picking up minor non-breaking fixes and not picking up significant changes could have stayed on the 0.10.* series. This would be in line with common understandings of version numbering (e.g. https://semver.org/ ).

Granted, you're technically not breaking semver due to "Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable." But this has been around for years and seems to be in pretty widespread use now :)

@srjennings
Copy link

All of a sudden chocolatey was breaking in Ansible plays. Took a bit to find this thread after stumbling through diagnostic logs. Definitely following this thread.

I echo @jborean93's assessment regarding breaking changes and being surprised by it.

@ferventcoder
Copy link
Member Author

FYI folks - for 0.10.14, we plan to switch the default to off. So folks can opt in.

@ferventcoder
Copy link
Member Author

@charlieok

Just to touch on the SemVer bits - we are semver compliant as we are still sub-v1. I think you are correct there should be a more significant bump in the sub v1 category to signify this. We do publish a changelog with how to turn things off if they do cause issues, we also have an announcements email at https://groups.google.com/group/chocolatey-announce where we publish these.

You are correct in that we should look to moving to a v1 release and it is something we have planned to do this year.

@ferventcoder
Copy link
Member Author

ferventcoder commented Apr 1, 2019

@jborean93 @nitzmahone apologies, I feel like we made a bad decision here in a move to get Chocolatey v0.10.12 out the door. So normally we would have had a lot more notice to folks about this as we did with #512 to allow time for preparing here. After more thought, I believe you are correct and this needs to be off by default and folks should opt-in to it. I'm not sure if we would want to switch the defaults at a later date either.

The nice thing about Chocolatey features is that we can switch defaults if a user has not explicitly made a change to them.

@ferventcoder
Copy link
Member Author

Folks, please follow #1784. The good news is we are going to have a fast turnaround for 0.10.14 due to this and a couple of other things that have been broken.

@nitzmahone
Copy link

@ferventcoder thanks- having been in similar situations on our own projects, we can definitely sympathize! Personally, I'm all for making the enhanced exit code behavior the default in a future Chocolatey release, once callers that are aware of the new behavior have had a reasonable amount of time to filter out into the world. Changing return values and exit codes is particularly tough, since you can't easily issue deprecation warnings on them (that aren't just noise to 99% of users, anyway).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants