-
Notifications
You must be signed in to change notification settings - Fork 645
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
[5.x] Improves console output and fixes Carbon v3 support #1387
Conversation
I think this broke the output of the commands, but I'm not sure. We restart Horizon every so often via a scheduled job and after the update from yesterday we've been getting these in production. So it's interpreting the status message as output instead of the status code? Can reproduce it. In one terminal shell: php artisan horizon In another: php artisan horizon:restart The exceptions:
Edit: created a new issue for it - #1388 |
I'm probably nitpicking but shouldn't this have been be Horizon 6.0 ? It changes PHP and Laravel requirements, plus the fact that it changes the output of the CLI commands means that some third party scripts could break. For example if I have a deployment pipeline that uses "horizon:status" and parses the output to make sure it's running, it would suddenly start throwing errors when upgrading from 5.22 to 5.23. |
I agree, this happened in our case and our tests didn't catch it because we had to mock the horizon command. |
Hey al. First of all I'm sorry this has disrupted you. That definitely wasn't our intention. About the version bumps: these are perfectly fine to make in minor releases. We do this all the time in our packages. Please see the rule here in the spec: https://semver.org/#spec-item-7 and here: https://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api I also want to note that we don't consider the output to be part of the public API and thus, in our eyes, there can be no breaking changes here. Again, we're sorry this disrupted you but we think we didn't do anything wrong here. |
This pull request does three things:
$date->addSeconds(...)
.Here is an example of the new console output:
Here is another:
And another: