Exceptions usage improvement, added directive error exception class.
Plain Expetion is replaced with TaskotronYamlError or TaskotronDirectiveError.
Details
Details
- Reviewers
kparal mkrizek - Maniphest Tasks
- T85: Improve Exceptions in Runner
- Commits
- rLTRNd962f5135cc2: Exceptions usage improvement, added directive error exception class.
python runtask.py -e xchat-2.8.8-21.fc20 -a x86_64 yaml_file
Where yaml_file is YAML file (e.g. rpmlint.yml from task-rpmlint repo) with
missing directive (e.g. rpmlint.yml with whole koji section missing) or
bad directive name (e.g. "oji" instead of "koji")
Diff Detail
Diff Detail
- Branch
- arcpatch-D21
- Lint
No Linters Available - Unit
No Unit Test Coverage
Comment Actions
I would use the same error when directive is not found or when it is misspelled - TaskotronYamlError. Also, please have a look at the remaining files as well, not just runner itself. Thanks.
Comment Actions
Looks good in general. Please fix the typos and don't forget to run tests.
| libtaskotron/directives/python_directive.py | ||
|---|---|---|
| 5 | This is a typo. You haven't run "py.test --functional", have you? | |
| libtaskotron/runner.py | ||
| 110–111 | Is this the same, just more readable? if not self.taskdata.get('input') or self.taskdata['input'].get('args') is None: | |
Comment Actions
$ py.test --functional =============================================================================================== test session starts =============================================================================================== platform linux2 -- Python 2.7.5 -- py-1.4.20 -- pytest-2.5.2 plugins: cov collected 46 items / 1 skipped testing/functest_python_directive.py ... testing/test_koji_directive.py .. testing/test_koji_utils.py ... testing/test_python_directive.py ........ testing/test_runner.py ....F.................. testing/test_taskyaml.py ....... ==================================================================================================== FAILURES ===================================================================================================== _____________________________________________________________________________ TestRunnerInputVerify.test_yamlrunner_fails_missing_arg _____________________________________________________________________________ self = <testing.test_runner.TestRunnerInputVerify instance at 0x1aa7440> def test_yamlrunner_fails_missing_arg(self): ref_cmd = '-e foo-1.2-3.fc99 footask.yml' ref_inputspec = {'input': 'other_item'} test_parser = runner.get_argparser() test_args = vars(test_parser.parse_args(ref_cmd.split())) test_runner = runner.Runner(ref_inputspec, test_args) with pytest.raises(TaskotronYamlError): > test_runner._validate_input() testing/test_runner.py:67: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <libtaskotron.runner.Runner instance at 0x1aa7638> def _validate_input(self): if 'input' not in self.taskdata: return > if not self.taskdata.get('input') or self.taskdata['input'].get('args') is None: E AttributeError: 'str' object has no attribute 'get' libtaskotron/runner.py:110: AttributeError ================================================================================= 1 failed, 45 passed, 1 skipped in 0.33 seconds ==================================================================================
Let @garretraziel deal with this in D20.
Comment Actions
Other than wondering whether a koji-specific exception is really a good idea, looks good to me
| libtaskotron/exceptions.py | ||
|---|---|---|
| 33 | Do we really want an exception specific to the koji client? This seems excessively specific to me and a TaskotronLibraryError or something more generic seems more appropriate | |
Comment Actions
I have talked to Josef and we agreed that it would be nice if all major user-facing methods (high level) would throw TaskotronError-inherited exceptions, so it's easy to handle them. The low level methods and functions can continue using other errors (either built-in or third-library related), but the high level methods which call them should wrap them into TaskotronError-inherited exceptions).
Lukáš, can you please go through the code and look at all the raise statements? Especially the empty raise statements could be a good candidates to consider wrapping an error into our version of an error.
Also, can you make sure that all methods potentially throwing any exceptions have it described in their docstring? Thanks.
| libtaskotron/exceptions.py | ||
|---|---|---|
| 33 | I'm not fully sure what TaskotronLibraryError would mean. Any errors from *_utils files? Or something else? I like the name, but I'm not sure of the scope. However, for the moment, I think we could introduce TaskotronRemoteError that would cover all network and remote-server related errors. So even if your query is wrong, or if there is a network failure, you would get this exception. What do you think? | |
| libtaskotron/koji_utils.py | ||
| 51 | Let's change the error message to something like: | |
| 74 | This line can throw urlgrabber.grabber.URLGrabError (see current method docstring). The download() method should be modified to throw TaskotronRemoteError (because it seems reasonable that the end-user could use that method) and the current docstring adjusted to reflect that. | |
| 91–92 | This is nasty. First, we're comparing directly against error message (which I just proposed to change, thus breaking this code), and second, even though we consider no rpms to be an error state in nvr_to_urls() (we throw an exception), we ignore it here. This should be made consistent. Last but not least, if there is any other Taskotron error not matching the string, we just silently ignore it and return the list. That's evil! :-) I propose to get rid of the whole try-except block. If something fails in get_nvr_rpms(), then TaskotronRemoteError will get raised and can be handled in the caller method. | |
Comment Actions
Interesting article here (kudos @jskladan):
http://eli.thegreenplace.net/2008/08/21/robust-exception-handling/
Comment Actions
- rising TaskotronError-inherited exceptions, removed TODOs, added remote error exception and generic taskotron library exception
| libtaskotron/exceptions.py | ||
|---|---|---|
| 37–39 | When I read the docstring, I'm not really sure what the difference is between TaskotronError and TaskotronLibraryError. This whole project is the taskotron library. Make we can get by without this error for the moment, and introduce it when we have a better idea what it should represent? If somebody has a clear idea of the distinction between these two, please speak up. | |
| libtaskotron/file_utils.py | ||
| 23–25 | Actually, I think raising OSError here was quite fine. I'm sorry for being a pest :-) This is probably a judgement call. My thinking is that when you call makedirs(), you usually expect that OSError can happen - it's that same situation as with os.makedirs(). However, if any higher level method/function uses this, it should handle the OSError and raise a TaskotronError-based exception. What we should definitely do, is to document this in the docstring. | |
| 91 | For logging errors, it's better to use log.exception(). Maybe something like this? log.exception('Download failed for: %s', url)That will also include the url in the error message. | |
| 95–96 | Again, log.exception() would also include the traceback (therefore reason) in here, might be useful for finding out the root cause of the problem. | |
| libtaskotron/koji_utils.py | ||
| 62 | This should be adjusted to TaskotronRemoteError | |
| 79–84 | Can you add a docstring specifying which errors can be thrown here? Thanks. | |
| requirements.txt | ||
| 1 ↗ | (On Diff #61) | This is already covered in the readme. We want to use system-level urlgrabber, not the one from pypi. (Also, we intend to move from it in the future). Please remove this line. |
| libtaskotron/file_utils.py | ||
|---|---|---|
| 23–25 | +1 on the OSError (since we directly state that it is the same. But please make sure to document the behaviour in the docstring. | |
| libtaskotron/file_utils.py | ||
|---|---|---|
| 9–12 | Please document raised exception. | |
| 63 | Please document raised exception. Also say that it occurs when the target directory/file can't be created. | |
| 76 | I think you should do log.exception("Can't create directory: %s' % dest")The exception message and traceback gets printed automatically. | |
| 77 | It might be sufficient to call TaskotronRemoteError(e), but you need to try it. Just a detail, not important. | |
| libtaskotron/koji_utils.py | ||
| 80 | When looking at the source code, I believe this downloads all RPMs of all NVRs tagged by a specific Koji tag. dest is the destination directory, arches is a list of architectures. | |
Comment Actions
@lbrabec: A hint for the next time: You don't need to close the review by hand, Phabricator will close it automatically shortly after you push the change (using arc land or git push, that doesn't matter). As a bonus, it will include a git commit hash and a link to the git web browser.
Also, please clean up the commit message a bit. This message:
Exceptions usage improvement, added directive error exception class. Summary: Exceptions usage improvement, added directive error exception class. Plain Expetion is replaced with TaskotronYamlError or TaskotronDirectiveError. Test Plan: python runtask.py -e xchat-2.8.8-21.fc20 -a x86_64 yaml_file Where yaml_file is YAML file (e.g. rpmlint.yml from task-rpmlint repo) with missing directive (e.g. rpmlint.yml with whole koji section missing) or bad directive name (e.g. "oji" instead of "koji") Reviewers: kparal, mkrizek Reviewed By: kparal CC: tflink, garretraziel, jskladan Maniphest Tasks: T85 Differential Revision: https://phab.qadevel.cloud.fedoraproject.org/D21
could have easily been stripped down to this, I think:
Exceptions usage improvement, added directive error exception class. Plain Expetion is replaced with TaskotronYamlError or TaskotronDirectiveError. Maniphest Tasks: T85 Differential Revision: https://phab.qadevel.cloud.fedoraproject.org/D21
(For this reason, I started to use plain git merge && git push, because arc land doesn't offer any additional benefit and it doesn't allow you to edit the commit message by default.)
Please grep TODO/T85 and remove them. Thanks.