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
[dev.icinga.com #12654] Exceptions on CLI should NOT show exception details #2546
Comments
Updated by jmeyer on 2016-09-06 09:13:33 +00:00 No. --trace has nothing to do with this and won't help anyone if the error is not that easily reproducible. What is actually required here, is a more advanced error handling for the CLI. I'm with you about handled errors, these should focus on being easy to read. But unhandled exceptions must include the file and line at the very least. What I'd like to see is the same what we currently have for forms:
These will emit easy to read and possibly colored messages. Command::fail() on the other hand should preferably not adjusted in any way and be handled like any other exception. For me it's a convenience method for throwing a new exception, nothing else. But that's open for discussion I guess. Whatever will happen with Command::fail(), we must ensure that all modules using this method are properly adjusted so that the new error handling is used instead. |
Updated by tgelf on 2016-09-06 10:14:51 +00:00 Meaningful exception messages are mostly unique or at least rarely distributed, so a quick grep should in most cases lead you to the desired target. If the message makes no sense it's a bug in the throwing class. I do not remember to have been interested in this information even once, and I write LOTS of CLI commands in Web 2 modules. And where the fail() shortcut is in use it is just absurd, as the provided information never makes any sense:
Sure, this could be fixed in fail(), but I'm not interested in this information at all. Never. More practical example, cronjob fails because of DB outage. A sysadmin wants to read...
...when something went wrong with the DB. And not something like this:
To better understand your reasoning: could you please show me one of our commands where full exception class name, filename and line make any sense? Regarding your other examples: that's what we use the Logger for. Sure, there could also be shortcuts - but that's completely unrelated to this issue. Cheers, |
Updated by jmeyer on 2016-09-06 11:58:00 +00:00 Ah well, yes, you're right. The Logger class is for beautiful error messages. Then scratch that. ;) The point of my previous comment was to clarify that I want to see the small stacktrace regardless whether --trace is passed or not, if an unhandled exception occurred. To be clear: The error handling in class Icinga\Cli\Loader was effectively masking every exception, whether handled or not. This must not be introduced again when fixing Command::fail(). |
Updated by jmeyer on 2016-10-04 11:51:35 +00:00
Well, I had a discussion a few weeks ago with Thomas but forgot what we "agreed" upon. However, our two standpoints were as follows:
A solution that considers both standpoints is still to be defined. |
This issue has been migrated from Redmine: https://dev.icinga.com/issues/12654
Created by tgelf on 2016-09-06 08:12:35 +00:00
Assignee: (none)
Status: New
Target Version: Backlog
Last Update: 2016-10-04 11:51:35 +00:00 (in Redmine)
Exceptions on CLI should NOT show exception details (neither class nor file or line) unless -
trace is passed. Especially not when the exception got triggered intentionally by issuing $this>fail() in a command action. Currently the filename is often longer than the real error message, when your output is wrapped or shortened (e.g. default system/journalctl output) you can not even read what really happened.Reasoning: CLI is for admins, they need readable error messages. Who wants to see a stack trace can add --trace.
The text was updated successfully, but these errors were encountered: