Skip to content
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.

Sign up for GitHub

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

Better handle errors in entry points #50632

Merged
merged 6 commits into from
Nov 30, 2018
+19 −3
Merged

Better handle errors in entry points #50632

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fb05bd5
Eat an exception from an entry point instead of letting it kill the s…
AstraLuma Nov 26, 2018
07868f1
Improve logging output
AstraLuma Nov 28, 2018
dadeb0d
Merge branch '2017.7' into loader-exception-handler
AstraLuma Nov 28, 2018
8697744
Use single quotes per style guide
AstraLuma Nov 30, 2018
194801a
Merge branch 'loader-exception-handler' of github.com:astronouth7303/…
AstraLuma Nov 30, 2018
0f72815
Merge branch '2017.7' into loader-exception-handler
AstraLuma Nov 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions salt/loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,12 @@ def _module_dirs(
ext_type_types.extend(opts[ext_type_dirs])
if HAS_PKG_RESOURCES and ext_type_dirs:
for entry_point in pkg_resources.iter_entry_points('salt.loader', ext_type_dirs):
loaded_entry_point = entry_point.load()
for path in loaded_entry_point():
ext_type_types.append(path)
try:
loaded_entry_point = entry_point.load()
for path in loaded_entry_point():
ext_type_types.append(path)
except Exception:
log.exception("Error running entry point %r", entry_point)
Copy link
Contributor

@isbm isbm Nov 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message needs to be rephrased for a regular DevOp, who has no idea what means "entry point" here and why it should run.

Also I believe it should be at error level. We want traceback only in debug mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll see what I can do. I'm not sure the names that the salt administrator would be familiar are readily available. I'll have to dig into the data types.

You know that exception is error level?

Copy link
Contributor

@isbm isbm Nov 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know that exception is error level?

At error. And it gives you a traceback. Do we actually need it? Your error is only informative that failed to run an entry point 😉 (please clarify what means those running entry points!). So I'd rather give a traceback here only when one is really looking at it:

try:
 ...
except MySpecificException as exc:
    log.error('An entry point just ran away from you!')
except Exception:
    log.error('Oh, this neatly crashed.')
    log.debug('This is how it crashed. Enjoy your day.', exc_info=True)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The developer would certainly strongly appreciate the traceback, and I would consider any time this code is triggered to be a bug in some third-party code.


cli_module_dirs = []
# The dirs can be any module dir, or a in-tree _{ext_type} dir
Expand Down