-
Notifications
You must be signed in to change notification settings - Fork 187
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
Fix exception handling in commands calling list_repository_contents
#3968
Conversation
The commands `verdi node repo ls` and `verdi calcjob inputls/outputls` were passing the caught exception object straight into the echo call without casting it to a string first, which would incur another exception by itself. On top of that, they were catching `ValueError` exceptions, which, as far as I can tell, would never be thrown. Instead if the path does not exist a `FileNotFoundError` will be thrown. The commands now catch this instead and print a useful message. This failure path of the commands was not tested, so the bug went unnoticed. The problem of raw exception objects being passed to `echo_critical` were present in some other commands as well, which have now been fixed.
Codecov Report
@@ Coverage Diff @@
## develop #3968 +/- ##
========================================
Coverage 78.33% 78.33%
========================================
Files 461 461
Lines 34076 34076
========================================
+ Hits 26692 26694 +2
+ Misses 7384 7382 -2
Continue to review full report at Codecov.
|
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.
thanks for the fixes and for adding the tests - some comments/questions
@@ -176,8 +176,8 @@ def calcjob_inputls(calcjob, path, color): | |||
|
|||
try: | |||
list_repository_contents(calcjob, path, color) | |||
except ValueError as exception: | |||
echo.echo_critical(exception) | |||
except FileNotFoundError: |
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.
the only comment I have is that this might become a different exception when we move to the new repository implementation.
is there perhaps a higher-level exception that we could catch here?
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.
The tests I have added will then exactly catch that change and we can adapt it. Don't see the point in trying to anticipate the type of exception that this might throw in the future. Or did you mean to catch a more generic exception like OSError
which is the base class of FileNotFoundError
?
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.
right, I meant more generic.
Anyhow, if you think we should just use the one we have currently it's also ok. the tests will fail if this changes, so it will be fixed when it changes.
@@ -71,7 +71,7 @@ def start(foreground, number): | |||
subprocess.check_output(command, env=currenv, stderr=subprocess.STDOUT) # pylint: disable=unexpected-keyword-arg | |||
except subprocess.CalledProcessError as exception: | |||
click.secho('FAILED', fg='red', bold=True) | |||
echo.echo_critical(exception.output) | |||
echo.echo_critical(str(exception)) |
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.
Or use repr
, see https://stackoverflow.com/a/4308203/1069467
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 used str
since that is what is intended for normal users, whereas repr
is for debugging purposes. I think in this case, showing the exception type for the user is not necessary. But if you think this would actually be better, happy to change 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 see... up to you.
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.
Looks good to me. Nice catch!
good to go from me as well |
Fixes #3967
The commands
verdi node repo ls
andverdi calcjob inputls/outputls
were passing the caught exception object straight into the echo call
without casting it to a string first, which would incur another
exception by itself. On top of that, they were catching
ValueError
exceptions, which, as far as I can tell, would never be thrown. Instead
if the path does not exist a
FileNotFoundError
will be thrown. Thecommands now catch this instead and print a useful message. This failure
path of the commands was not tested, so the bug went unnoticed.
The problem of raw exception objects being passed to
echo_critical
were present in some other commands as well, which have now been fixed.