----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20687/#review41612 -----------------------------------------------------------
src/main/python/apache/aurora/client/cli/__init__.py <https://reviews.apache.org/r/20687/#comment75134> Refactor this code into a separate function. src/main/python/apache/aurora/client/cli/__init__.py <https://reviews.apache.org/r/20687/#comment75135> refactoring this logic like _run_pre_command into a separate function will improve the readability further. src/main/python/apache/aurora/client/cli/__init__.py <https://reviews.apache.org/r/20687/#comment75136> Shouldn't this be logged as logging.ERROR? src/main/python/apache/aurora/client/cli/__init__.py <https://reviews.apache.org/r/20687/#comment75137> Add a new line before the try block. Also, this method is too big, consider refactoring this method into smaller methods. I think it will also improve the testability of this code. src/main/python/apache/aurora/client/cli/command_hooks.py <https://reviews.apache.org/r/20687/#comment75142> Please add a class doc here. src/main/python/apache/aurora/client/cli/command_hooks.py <https://reviews.apache.org/r/20687/#comment75138> should the name be prefixed with a . or make the name all caps? src/main/python/apache/aurora/client/cli/command_hooks.py <https://reviews.apache.org/r/20687/#comment75141> Include more doc about what a hooks file is. Is it a python file? Can it be any python file? A pointer to a sample file will he helpful here. src/main/python/apache/aurora/client/cli/command_hooks.py <https://reviews.apache.org/r/20687/#comment75139> I think we should resolve this before we ship this change. src/main/python/apache/aurora/client/cli/command_hooks.py <https://reviews.apache.org/r/20687/#comment75145> change to print_err? src/main/python/apache/aurora/client/cli/command_hooks.py <https://reviews.apache.org/r/20687/#comment75140> inline src/main/python/apache/aurora/client/cli/command_hooks.py <https://reviews.apache.org/r/20687/#comment75144> inline src/test/python/apache/aurora/client/cli/test_command_hooks.py <https://reviews.apache.org/r/20687/#comment75148> Is it possible to add a file that has compile and execution errors to test? Ensuring that the logging and error handling works as expected will simplify writing and debugging hooks. - Suman Karumuri On April 25, 2014, 12:17 a.m., Mark Chu-Carroll wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20687/ > ----------------------------------------------------------- > > (Updated April 25, 2014, 12:17 a.m.) > > > Review request for Aurora, David McLaughlin and Suman Karumuri. > > > Bugs: aurora-270 > https://issues.apache.org/jira/browse/aurora-270 > > > Repository: aurora > > > Description > ------- > > Stage 1 of implementing command hooks for aurora v2. > > This change includes: > (1) The ability to add hard-wired hooks, by registering them in > ConfigurationPlugins > compiled into a pex; > (2) Dynamically loaded plugins, loaded from plugin files. > > The dynamically loaded plugins are *not* currently active outside of tests. > > The second stage of this change will activate dynamically loaded plugins, and > provide a mechanism to allow privileged users to override hooks. > > > Diffs > ----- > > src/main/python/apache/aurora/client/cli/BUILD > 17cdc287875b5f0832064a6441f33fc9837fc79b > src/main/python/apache/aurora/client/cli/__init__.py > 5a10328e49f0128965aed73b9c167324dfcfde0f > src/main/python/apache/aurora/client/cli/command_hooks.py PRE-CREATION > src/main/python/apache/aurora/client/cli/jobs.py > 0534bdf72a332caa606dd3a7ca743a59e03738ef > src/test/python/apache/aurora/client/cli/AuroraHooks PRE-CREATION > src/test/python/apache/aurora/client/cli/BUILD > 34fdb47baa647b9c3bd149ff2710b175c7435dae > src/test/python/apache/aurora/client/cli/test_command_hooks.py PRE-CREATION > > Diff: https://reviews.apache.org/r/20687/diff/ > > > Testing > ------- > > [sun-wukong incubator-aurora (command_hooks)]$ ./pants > src/test/python/apache/aurora/client/cli:all > Build operating on targets: > OrderedSet([PythonTestSuite(src/test/python/apache/aurora/client/cli/BUILD:all)]) > ================================================= test session starts > ================================================== > platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2 > collected 4 items > > src/test/python/apache/aurora/client/cli/test_bridge.py .... > > =============================================== 4 passed in 0.03 seconds > =============================================== > ================================================= test session starts > ================================================== > platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2 > collected 4 items > > src/test/python/apache/aurora/client/cli/test_command_hooks.py .... > > =============================================== 4 passed in 0.58 seconds > =============================================== > ================================================= test session starts > ================================================== > platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2 > collected 5 items > > src/test/python/apache/aurora/client/cli/test_help.py ..... > > =============================================== 5 passed in 0.52 seconds > =============================================== > ================================================= test session starts > ================================================== > platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2 > collected 36 items > > src/test/python/apache/aurora/client/cli/test_cancel_update.py .. > src/test/python/apache/aurora/client/cli/test_create.py .... > src/test/python/apache/aurora/client/cli/test_diff.py ... > src/test/python/apache/aurora/client/cli/test_kill.py ......... > src/test/python/apache/aurora/client/cli/test_open.py ..... > src/test/python/apache/aurora/client/cli/test_restart.py ... > src/test/python/apache/aurora/client/cli/test_status.py ....... > src/test/python/apache/aurora/client/cli/test_update.py ... > > ============================================== 36 passed in 1.87 seconds > =============================================== > ================================================= test session starts > ================================================== > platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2 > collected 1 items > > src/test/python/apache/aurora/client/cli/test_logging.py . > > =============================================== 1 passed in 0.62 seconds > =============================================== > ================================================= test session starts > ================================================== > platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2 > collected 2 items > > src/test/python/apache/aurora/client/cli/test_plugins.py .. > > =============================================== 2 passed in 0.53 seconds > =============================================== > ================================================= test session starts > ================================================== > platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2 > collected 4 items > > src/test/python/apache/aurora/client/cli/test_quota.py .... > > =============================================== 4 passed in 0.55 seconds > =============================================== > ================================================= test session starts > ================================================== > platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2 > collected 5 items > > src/test/python/apache/aurora/client/cli/test_sla.py ..... > > =============================================== 5 passed in 0.56 seconds > =============================================== > ================================================= test session starts > ================================================== > platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2 > collected 2 items > > src/test/python/apache/aurora/client/cli/test_task_run.py .. > > =============================================== 2 passed in 0.54 seconds > =============================================== > src.test.python.apache.aurora.client.cli.bridge > ..... SUCCESS > src.test.python.apache.aurora.client.cli.command_hooks > ..... SUCCESS > src.test.python.apache.aurora.client.cli.help > ..... SUCCESS > src.test.python.apache.aurora.client.cli.job > ..... SUCCESS > src.test.python.apache.aurora.client.cli.logging > ..... SUCCESS > src.test.python.apache.aurora.client.cli.plugins > ..... SUCCESS > src.test.python.apache.aurora.client.cli.quota > ..... SUCCESS > src.test.python.apache.aurora.client.cli.sla > ..... SUCCESS > src.test.python.apache.aurora.client.cli.task > ..... SUCCESS > [sun-wukong incubator-aurora (command_hooks)]$ > > > Thanks, > > Mark Chu-Carroll > >
