Correctly propagate exceptions to exit code #854
Conversation
| @@ -517,4 +517,4 @@ def main(argv=None): | |||
| logging.getLogger('neo4j.bolt').setLevel(logging.WARNING) | |||
| argv = argv if argv is not None else sys.argv[1:] | |||
| default_sync = cartography.sync.build_default_sync() | |||
| return CLI(default_sync, prog='cartography').main(argv) | |||
| sys.exit(CLI(default_sync, prog='cartography').main(argv)) | |||
There was a problem hiding this comment.
This is the main change of this PR.
Kinda funny that our dev docs say to do sys.exit() but our code has been doing this incorrectly all this time: https://lyft.github.io/cartography/dev/developer-guide.html#implementing-custom-sync-commands.
There was a problem hiding this comment.
cc: @ryan-lane @jg10
| """ | ||
| Entrypoint for the command line interface. | ||
|
|
||
| :type argv: string | ||
| :param argv: The parameters supplied to the command line program. | ||
| """ | ||
| # TODO support parameter lookup in environment variables if not present on command line | ||
| config: cartography.config.Config = self.parser.parse_args(argv) | ||
| config: argparse.Namespace = self.parser.parse_args(argv) |
There was a problem hiding this comment.
We do some duck typing with cartography.config.Config so that it has the same fields as the object returned by parse_args(), but teeechnically parse_args() returns an argparse.Namespace object so I'll type-annotate it properly here.
|
|
||
|
|
||
| def run_with_config(sync, config): | ||
| def run_with_config(sync: Sync, config: Union[Config, argparse.Namespace]) -> int: |
There was a problem hiding this comment.
Again, our duck typing implicitly handled the cartography Config object vs argparse Namespace arg here (which was pretty neat), but explicit is better
Currently if any of these errors happen (e.g. we fail to connect to the neo4j database) then we return 1 and the exit code is not propagated up to the process -- the resulting returned exit code to the OS is still 0 (success). This PR fixes this and adds typehints to make this process a bit easier to read.
Related to #509.