-
Notifications
You must be signed in to change notification settings - Fork 656
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
refactoring: plugins can report their template dir #2300
Conversation
Fix problem where location of templates was hardcoded. Signed-off-by: Sorin Sbarnea <ssbarnea@redhat.com>
_instance.execute() | ||
|
||
assert 1 == e.value.code |
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.
We should still be providing something to the user in the case of a KeyError
instead of a stack trace!?
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.
@decentral1se You would be surprised but in this case the stack trace is more useful than the previous error message which was totally misleading: referring to missing folder, not inexistent driver.
Try it, that one is very easy to test from cli, before and after: molecule init role --driver-name foo
.
Usually I am for clear error messages, but a nice error message here would require a wide range of changes. Should be done eventually but not part of this change.
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.
OK, sure. However, there is now an increasing list of "things for the future". Please start to mark them in the code! Add a TODO, XXX, whatever or create an issue. If you forget, nobody will remember to do these things until the users have to run into them.
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.
There where already there, the difference is that now we uncover them.
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 see no TODO in this change or added by you. So no, they are not there.
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.
Please document somewhere that we need to loop back on this stuff to clean up.
Fix problem where location of templates was hardcoded.