RFC: Return with exit code 1 for some commands when an error occurs #3577

Closed
danielbachhuber opened this Issue Nov 16, 2016 · 17 comments

Projects

None yet
@danielbachhuber
Member
danielbachhuber commented Nov 16, 2016 edited

Background

Some commands support performing the same operation against multiple resources (e.g. updating two or more plugins wp plugin update akismet hello). If one of the operations fails (e.g. a plugin can't be installed), the command will display a warning, continue on, and exit with return code 0.

salty-wordpress ➜  wordpress-develop.dev  wp plugin install foobar edit-flow akismet
Warning: Couldn't find 'foobar' in the WordPress.org plugin directory.
Warning: edit-flow: An unexpected error occurred. Something may be wrong with WordPress.org or this server&#8217;s configuration. If you continue to have problems, please try the <a href="https://wordpress.org/support/">support forums</a>.
Warning: akismet: Plugin already installed.
salty-wordpress ➜  wordpress-develop.dev  echo $?
0

There have been a few long-outstanding issues on this topic (#2002, #1430, #427). If we're going to change the behavior, the time to change the behavior is before WP-CLI v1.0.0.

Proposal

For the commands that perform the same operation against multiple resources, WP-CLI will exit with return code 1 if one or more of the operations failed.

The affected commands include:

  • wp media (regenerate|import)
  • wp menu delete
  • wp menu item delete
  • wp plugin (install|activate|update|toggle|deactivate|uninstall|delete)
  • wp super-admin add
  • wp theme (install|update)
  • wp term delete
  • wp widget (delete|deactivate|reset)

Rationale

For WP-CLI users who have integrated these commands into provisioning systems and other automation, return codes provide a helpful, machine-readable description of the results of the operation. Exiting with return code 1 will inform the system that some part of the operation has failed.

I'm most interested in feedback from WP-CLI users who have incorporated these commands into automated systems. If there are serious backwards compatibility concerns established, then we'll keep the existing behavior and make sure it's explicitly documented. Otherwise, the proposal will be accepted, we'll change the behavior of the mentioned commands, and document the new behavior.

The decision on this proposal will be made on Tuesday, November 22nd.

@danielbachhuber danielbachhuber changed the title from RFC: Change exit code to 1 for some commands when an error occurs to RFC: Return with exit code 1 for some commands when an error occurs Nov 16, 2016
@danielbachhuber danielbachhuber added this to the 1.0.0 milestone Nov 16, 2016
@voldemortensen
Contributor

This would require quite a few changes to tools at Bluehost. They'd probably be relatively easy changes, but time consuming to go hunt everything down.

@danielbachhuber
Member

This would require quite a few changes to tools at Bluehost.

@voldemortensen In the interest of helping to produce an informed decision, could you spend a bit of time tracking down a couple of specific examples?

@JayWood
JayWood commented Nov 16, 2016

@danielbachhuber to be clear, are we talking a rewrite of WP_CLI::warning() to exit with 1? Or just the above mentioned commands?

@markng
markng commented Nov 16, 2016

This would affect Liquid Web minimally (enough that we'd appreciate the warning), and the change would help us in the future.

@danielbachhuber
Member

to be clear, are we talking a rewrite of WP_CLI::warning() to exit with 1? Or just the above mentioned commands?

The latter. I'd change the behavior of each existing command. WP_CLI::warning() won't be changed; in fact, there are cases where we still want to display a warning without changing the return code.

@Ipstenu
Ipstenu commented Nov 16, 2016

We chain commands at DreamHost, instead of an all in one, so that shouldn't impact us except in places where we're interactively running them (like a person would actually see the error). I'm triple checking to be sure though :)

@jeichorn
Contributor

Seems like a reasonable change, I can't think of any cases where this would cause us problems at Pagely

@greg-1-anderson
Contributor

LGTM. New behavior is highly desirable, and shouldn't cause any problems at Pantheon.

@westonruter
Contributor

Wholeheartedly agree with this change. This will allow the checks to be used in shell conditionals. Something to look for when check for scripts to update is for any scripts that include set -e. Before this change, a WP-CLI exit failure would not cause the script to exit at that point. Now, however, scripts that use set -e will need to explicitly acknowledge failures, like:

set -e
# ...
if ! wp post generate; then
    echo "Failed to generate posts, but oh well. Keep going!"
fi
# ...
@jonathanbardo
Member

I think this is definitely the way to go and at first look will have minimal impact on our side at GoDaddy.

@schlessera
schlessera commented Nov 17, 2016 edited

WP-CLI will exit with return code 1 if one or more of the operations failed.

Did you think about having variable return codes to provide more context to the caller?

For example, you can differentiate between partial failures (two of the seven plugins did not install) and complete failures (no write access to install plugins), which allows the subsequent tools to decide how to proceed. For a partial failure, additional cleanup work / a rollback might be needed.

The scripts that don't care about this additional information can still just check for a non-zero exit code, the additional information is optional.

For reference, here's the list of exit codes with special meanings, that shouldn't be used: http://www.tldp.org/LDP/abs/html/exitcodes.html

@danielbachhuber
Member

Did you think about having variable return codes to provide more context to the caller?

I did. At this point, I'd much prefer to keep things simple. If we introduce variable return codes in this context, then all WP-CLI commands will need to be audited to ensure they're using the "correct" return code. Binary is much more straightforward.

@gmcmillan

As someone who has this integrated into provisioning systems, I would much prefer the new behavior. If anything, this will improve our provisioning as we can better know when a failure happens and respond accordingly.

IMO even for interactive use of wp-cli, the exit code should return 1 or something non-zero if only 1 out of the 2 plugin installations succeeded in that scenario. That is a failure in my book because the operation being requested (install 2 plugins -- not maybe_install 2 plugins) has failed.

Having visibility into which operation failed is important. If we are installing 10 plugins and the only thing we get back is a return code of 1, it would be helpful to have information about which of those operations failed (and maybe even which, if any, succeeded)? Perhaps that can be sent to stderr?

@greg-1-anderson
Contributor

Opened #3580 to discuss gmcmilan's suggestion.

@danielbachhuber
Member

If we are installing 10 plugins and the only thing we get back is a return code of 1, it would be helpful to have information about which of those operations failed (and maybe even which, if any, succeeded)? Perhaps that can be sent to stderr?

This information is already sent to STDERR in the form of Warning::

salty-wordpress ➜  wordpress-develop.dev  wp plugin install foobar edit-flow akismet
Warning: Couldn't find 'foobar' in the WordPress.org plugin directory.
Warning: edit-flow: An unexpected error occurred. Something may be wrong with WordPress.org or this server&#8217;s configuration. If you continue to have problems, please try the <a href="https://wordpress.org/support/">support forums</a>.
Warning: akismet: Plugin already installed.
@voldemortensen
Contributor

It seems I misremembered some code and I don't think adding this functionality would effect us.

@danielbachhuber
Member

Thanks, everyone, for taking the time to weigh in on this. I'm moving forward with the proposal.

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