-
-
Notifications
You must be signed in to change notification settings - Fork 740
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
Pack Config Jinja render failures do not provide good tracebacks #4123
Comments
Here are my findings on this issue: As of May 17, 2019, exception thrown is
I believe this exception change is due to the commit 8096ba7 If needed, here is something we can do to improve further if needed.
This way all Jinja template related errors will be caught specifically. With the above code in place, we will get the following output:
Let me know if this is acceptable and I can update the code and submit. |
@VineeshJain I can't tell the difference in the |
@m4dcoder Yes mainly that is the difference. Original issue mentioned that TemplateSyntaxError can only take 3 arguments. TemplateError is the base class for all Jinja errors which takes 2 arguments. Also, I feel it is better that we catch TemplateError for all jinja related errors and catch a generic Exception for all other types of exceptions. That way it is clearly identified that the issue is related to Jinja or due to any other issue. Also, the error message is defined by us, we can change it to anything we want, let me know if you think the message is not good enough. Keep in mind though, we should have a cover-all sought of message for all TemplateErrors and not specific to TemplateSyntaxError. We can catch the TemplateSyntaxError specifically as well and raise is as is without modifying it at all. |
IMO, no additional change is required since there is already a fix committed. The specific error type is already included in the error message. There's no new information in this proposal. Otherwise we will have to keep expanding the try-except block to add more specific error types. |
Looks like i forgot to close the issue after i fixed it last year. Closing. |
When working on #4121 i found another bug in the code.
Steps to reproduce
st2 pack install email
/opt/stackstorm/packs/email.yaml
with an invalid jinja expression in itst2ctl reload --register-configs
Expected Behavior
The code https://github.com/StackStorm/st2/blob/master/st2common/st2common/util/config_loader.py#L192 should have re-thrown the underlying Jinja exception (
jinja2.exceptions.TemplateSyntaxError
). However, this exception signature takes a different number of parameters than what's being passed in: https://github.com/pallets/jinja/blob/master/jinja2/exceptions.py#L84Since the code is only passing in a message and leaving out the
lineno
argument, a different exception is thrown for an invalid call. This defeats the purpose of catching/re-throwing in the first place.Thoughts on a fix
I don't see many "good" ways to fix this that would be future proof while continuing to throw the underlying exception. My only thought would be to wrap up the exception details, add that to the message and throw a new exception type that we know and can control the parameters of.
The text was updated successfully, but these errors were encountered: