Re: Review Request 27852: Ensure run verb returns an exit code.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27852/ --- (Updated Nov. 14, 2014, 2:15 p.m.) Review request for Aurora and Bill Farner. Changes --- Bill's feedback. Bugs: AURORA-923 https://issues.apache.org/jira/browse/AURORA-923 Repository: aurora Description --- Ensure run verb returns an exit code. Diffs (updated) - src/main/python/apache/aurora/client/cli/__init__.py 6e553d8af459e575b2d62282a3bc0d1e266203d8 src/main/python/apache/aurora/client/cli/task.py 91175facdc8c9fd59ab66781f86ee8b5940a src/test/python/apache/aurora/client/cli/BUILD e1f9ebf96774b8f5c75de8570c6ba87d953ab649 src/test/python/apache/aurora/client/cli/test_kill.py e3a366bf67074e50787394cad58d5e01359b641e src/test/python/apache/aurora/client/cli/test_task_run.py 8d9ef0543c1ab514d6f039ba63a1d417a4a90a1b Diff: https://reviews.apache.org/r/27852/diff/ Testing --- ./pants build src/test/python/apache/aurora/client/cli:: Thanks, Zameer Manji
Re: Review Request 27852: Ensure run verb returns an exit code.
On Nov. 11, 2014, 2:55 p.m., Bill Farner wrote: src/main/python/apache/aurora/client/cli/task.py, line 72 https://reviews.apache.org/r/27852/diff/1/?file=757426#file757426line72 Should we just make this the default behavior? There's at least 31 locations that do this, seems like a catch-all would be useful. If we do this, i'd recommend you remove all `return EXIT_OK` lines, so only proceed if you're okay with that. Looks like the relevant changes would be in: src/main/python/apache/aurora/client/cli/standalone_client.py src/main/python/apache/aurora/client/cli/client.py Zameer Manji wrote: I don't see how this approach will work. This problem of not returning an exit code comes from the subclasses not implementing the method correctly not that we dispatch to the super class's implementation. In addition I'm not comfortable mixing in this (minor) change with a larger structural change. Bill Farner wrote: Are we talking about different things? I'm just suggesting a change like this, around sys.exit: ``` client = AuroraCommandLine() if len(sys.argv) == 1: sys.argv.append(help) - sys.exit(client.execute(sys.argv[1:])) + exit_code = client.execute(sys.argv[1:]) + sys.exit(0 if exit_code is None else exit_code) if __name__ == '__main__': proxy_main() ``` This is basically just changing the contract to exit 0 if no explicit exit code is returned. Given we're working in a language that doesn't have a compiler to enforce a return value, i think this is the right thing to do. The python philisohpy is 'explicit over implicit'. I feel that returning None is an error (that we should catch in our tests) and we should explicitly returning the exit code. - Zameer --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27852/#review60888 --- On Nov. 10, 2014, 6:50 p.m., Zameer Manji wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27852/ --- (Updated Nov. 10, 2014, 6:50 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-923 https://issues.apache.org/jira/browse/AURORA-923 Repository: aurora Description --- Ensure run verb returns an exit code. Diffs - src/main/python/apache/aurora/client/cli/task.py 91175facdc8c9fd59ab66781f86ee8b5940a src/test/python/apache/aurora/client/cli/BUILD e1f9ebf96774b8f5c75de8570c6ba87d953ab649 src/test/python/apache/aurora/client/cli/test_task_run.py 8d9ef0543c1ab514d6f039ba63a1d417a4a90a1b Diff: https://reviews.apache.org/r/27852/diff/ Testing --- ./pants build src/test/python/apache/aurora/client/cli:: Thanks, Zameer Manji
Re: Review Request 27852: Ensure run verb returns an exit code.
On Nov. 11, 2014, 10:55 p.m., Bill Farner wrote: src/main/python/apache/aurora/client/cli/task.py, line 72 https://reviews.apache.org/r/27852/diff/1/?file=757426#file757426line72 Should we just make this the default behavior? There's at least 31 locations that do this, seems like a catch-all would be useful. If we do this, i'd recommend you remove all `return EXIT_OK` lines, so only proceed if you're okay with that. Looks like the relevant changes would be in: src/main/python/apache/aurora/client/cli/standalone_client.py src/main/python/apache/aurora/client/cli/client.py Zameer Manji wrote: I don't see how this approach will work. This problem of not returning an exit code comes from the subclasses not implementing the method correctly not that we dispatch to the super class's implementation. In addition I'm not comfortable mixing in this (minor) change with a larger structural change. Bill Farner wrote: Are we talking about different things? I'm just suggesting a change like this, around sys.exit: ``` client = AuroraCommandLine() if len(sys.argv) == 1: sys.argv.append(help) - sys.exit(client.execute(sys.argv[1:])) + exit_code = client.execute(sys.argv[1:]) + sys.exit(0 if exit_code is None else exit_code) if __name__ == '__main__': proxy_main() ``` This is basically just changing the contract to exit 0 if no explicit exit code is returned. Given we're working in a language that doesn't have a compiler to enforce a return value, i think this is the right thing to do. Zameer Manji wrote: The python philisohpy is 'explicit over implicit'. I feel that returning None is an error (that we should catch in our tests) and we should explicitly returning the exit code. Works for me, in that case, let's own up and change the code i cited above to assert not None. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27852/#review60888 --- On Nov. 11, 2014, 2:50 a.m., Zameer Manji wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27852/ --- (Updated Nov. 11, 2014, 2:50 a.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-923 https://issues.apache.org/jira/browse/AURORA-923 Repository: aurora Description --- Ensure run verb returns an exit code. Diffs - src/main/python/apache/aurora/client/cli/task.py 91175facdc8c9fd59ab66781f86ee8b5940a src/test/python/apache/aurora/client/cli/BUILD e1f9ebf96774b8f5c75de8570c6ba87d953ab649 src/test/python/apache/aurora/client/cli/test_task_run.py 8d9ef0543c1ab514d6f039ba63a1d417a4a90a1b Diff: https://reviews.apache.org/r/27852/diff/ Testing --- ./pants build src/test/python/apache/aurora/client/cli:: Thanks, Zameer Manji
Re: Review Request 27852: Ensure run verb returns an exit code.
On Nov. 11, 2014, 2:55 p.m., Bill Farner wrote: src/main/python/apache/aurora/client/cli/task.py, line 72 https://reviews.apache.org/r/27852/diff/1/?file=757426#file757426line72 Should we just make this the default behavior? There's at least 31 locations that do this, seems like a catch-all would be useful. If we do this, i'd recommend you remove all `return EXIT_OK` lines, so only proceed if you're okay with that. Looks like the relevant changes would be in: src/main/python/apache/aurora/client/cli/standalone_client.py src/main/python/apache/aurora/client/cli/client.py I don't see how this approach will work. This problem of not returning an exit code comes from the subclasses not implementing the method correctly not that we dispatch to the super class's implementation. In addition I'm not comfortable mixing in this (minor) change with a larger structural change. - Zameer --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27852/#review60888 --- On Nov. 10, 2014, 6:50 p.m., Zameer Manji wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27852/ --- (Updated Nov. 10, 2014, 6:50 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-923 https://issues.apache.org/jira/browse/AURORA-923 Repository: aurora Description --- Ensure run verb returns an exit code. Diffs - src/main/python/apache/aurora/client/cli/task.py 91175facdc8c9fd59ab66781f86ee8b5940a src/test/python/apache/aurora/client/cli/BUILD e1f9ebf96774b8f5c75de8570c6ba87d953ab649 src/test/python/apache/aurora/client/cli/test_task_run.py 8d9ef0543c1ab514d6f039ba63a1d417a4a90a1b Diff: https://reviews.apache.org/r/27852/diff/ Testing --- ./pants build src/test/python/apache/aurora/client/cli:: Thanks, Zameer Manji
Re: Review Request 27852: Ensure run verb returns an exit code.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27852/#review60888 --- src/main/python/apache/aurora/client/cli/task.py https://reviews.apache.org/r/27852/#comment102294 Should we just make this the default behavior? There's at least 31 locations that do this, seems like a catch-all would be useful. If we do this, i'd recommend you remove all `return EXIT_OK` lines, so only proceed if you're okay with that. Looks like the relevant changes would be in: src/main/python/apache/aurora/client/cli/standalone_client.py src/main/python/apache/aurora/client/cli/client.py - Bill Farner On Nov. 11, 2014, 2:50 a.m., Zameer Manji wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27852/ --- (Updated Nov. 11, 2014, 2:50 a.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-923 https://issues.apache.org/jira/browse/AURORA-923 Repository: aurora Description --- Ensure run verb returns an exit code. Diffs - src/main/python/apache/aurora/client/cli/task.py 91175facdc8c9fd59ab66781f86ee8b5940a src/test/python/apache/aurora/client/cli/BUILD e1f9ebf96774b8f5c75de8570c6ba87d953ab649 src/test/python/apache/aurora/client/cli/test_task_run.py 8d9ef0543c1ab514d6f039ba63a1d417a4a90a1b Diff: https://reviews.apache.org/r/27852/diff/ Testing --- ./pants build src/test/python/apache/aurora/client/cli:: Thanks, Zameer Manji
