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

Return an error code to the shell if an error occurs when running platform server #8189

Merged
merged 1 commit into from Feb 2, 2018

Conversation

kemenaran
Copy link
Contributor

Summary

Currently, if an error occurs when starting the server using platform server, the command exit with a 0 exit code (i.e. "success") – although the server failed to launch.

This means it is not possible to know whether the server started successfully or not. From the shell point of view, it started successfully and existed very quickly. Additionally, no error message is printed to the console.

$ rm config/config.json
$ if bin/platform server; then echo "Success"; else echo "Failure"; fi
Success
# The server actually failed to launch, but there is no way to know it (besides a message in the log file)

With this change, the platform server command simply reports its errors to cobra. When an error occurs while initializing the app (like a missing or invalid configuration file, or missing locales), cobra prints the error to the console, and the shell command exits with a "-1" exit code.

This allow shell scripts to properly detect the startup failure, and to react to it.

$ rm config/config.json
$ if bin/platform server; then echo "Success"; else echo "Failure"; fi
Error: LoadConfig: Error decoding config file=config.json, err=While parsing config: invalid character ':' after top-level value,
Failure
# The server failed to launch, and it is possible to react to it in a script.
# (The error message is still added to the log file, but also printed
# on the console, as part of cobra's standard behavior.)

More importantly, in production, it allows the service launcher (like init or systemd) to know whether the server launched successfully, or if it needs to try again, how many times (depending on the service configuration), and so on.

Ticket Link

n/a

Checklist

  • Has enterprise changes (please link)
  • Has UI changes
  • Includes text changes and localization file (.../i18n/en.json updates
  • Touches critical sections of the codebase (auth, upgrade, etc.)

When starting the server using `platform server`, errors occuring during
startup are not reported in the console. The command exit with a 0 exit
code (i.e. "success"), although the server failed to launch.

With this change, when an error occurs while initializing the app (like
a missing or invalid configuration file):

- the error is printed to the console;
- the command exit with a "-1" exit code.

This allow shell scripts to properly detect the startup failure, and to
react to it.

Example of error displayed:

```
$ platform server
Error: LoadConfig: Error decoding config file=config.json, err=While parsing config: invalid character ':' after top-level value,
``
@mattermod
Copy link
Contributor

Thanks @kemenaran for the pull request!

Per the CONTRIBUTING.md file displayed when you created this pull request, we need to add you to the list of approved contributors for the Mattermost project.

Please help complete the Mattermost contribution license agreement?

This is a standard procedure for many open source projects. Your form should be processed within 24 hours and reviewers for your pull request will be able to proceed.

Please let us know if you have any questions.

We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team.

Use: "server",
Short: "Run the Mattermost server",
RunE: runServerCmd,
SilenceUsage: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SilentUsage: true simply avoids the command usage to be printed below the error message when a startup error occurs.

It doesn't affect other behaviors: platform server --help and platform server --xxx-invalid-option still display the command usage properly.

@jasonblais jasonblais added the 2: Dev Review Requires review by a developer label Feb 2, 2018
Copy link
Member

@crspeller crspeller left a comment

Choose a reason for hiding this comment

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

Looks good!

@ccbrown ccbrown merged commit 07902b4 into mattermost:master Feb 2, 2018
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Feb 2, 2018
@kemenaran
Copy link
Contributor Author

@ccbrown great, thanks for accepting this!

@lindalumitchell lindalumitchell added the Tests/Not Needed New release tests not required label Feb 6, 2018
@kemenaran kemenaran deleted the send-shell-exit-code branch February 7, 2018 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a developer Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Not Needed New release tests not required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants