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
Show an error message for the title when it would otherwise result in a 500 #864
Conversation
app/models/shipit/task.rb
Outdated
@@ -228,6 +228,8 @@ def supports_rollback? | |||
|
|||
def title | |||
definition.render_title(env) | |||
rescue KeyError | |||
'This task cannot be shown due to an incorrect variable name. Check your shipit.yml file' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\nit I would include the name of the task instead of referring to this
(if that is possible of course)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would have rescued in render_title
itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if we could make the error message more prominent. Could we catch something in the stacks controller and flash
it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to handle this one layer lower, but no huge deal.
Other than that LGTM.
app/models/shipit/task.rb
Outdated
@@ -228,6 +228,8 @@ def supports_rollback? | |||
|
|||
def title | |||
definition.render_title(env) | |||
rescue KeyError | |||
'This task cannot be shown due to an incorrect variable name. Check your shipit.yml file' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would have rescued in render_title
itself.
I pushed the implementation lower and changed the error message as suggested. Catching an exception in the controller would halt the execution though, so other tasks won't show. Avoiding that will require a lot more code/complexity than this error warrants I think, so I'll keep it as is. |
note, that deploy was a mistake. I didn't cut a new release so the deploy did not push to rubygems |
Fixes #862.
I opted to do a very simple error message here instead of something more fancy since it doesn't seem worth the added complexity. I could also have put this fix in the TaskDefinition and delegated to description again, but that would just hide the problem.