----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41428/#review110741 -----------------------------------------------------------
src/main/python/apache/aurora/client/config.py (line 103) <https://reviews.apache.org/r/41428/#comment170757> Try to avoid abbreviations in variable names. Just call this `shell_health_checker`? src/main/python/apache/aurora/client/config.py (lines 103 - 105) <https://reviews.apache.org/r/41428/#comment170758> Nit, could probably inline all of this: if not health_check_end_config.get(SHELL_HEALTH_CHECK, {}).get('shell_command'): die(...) src/main/python/apache/aurora/config/schema/base.py (line 59) <https://reviews.apache.org/r/41428/#comment170759> We should also rename `endpoint_config` to `health_checker`. And probably rename the above `DefaultEndpointConfig` to `DefaultHealthChecker` src/test/python/apache/aurora/client/test_config.py (line 187) <https://reviews.apache.org/r/41428/#comment170760> Kill this comment? It seems superfluous. src/test/python/apache/aurora/client/test_config.py (lines 236 - 239) <https://reviews.apache.org/r/41428/#comment170762> I'm somewhat uncomfortable with using monkeypatch to accomplish this. We had generally been trending towards adding injection points for mocks, rather than monkeypatching globals. How would you feel about adding a logger arg to `_validate_health_check_config` that just defaults to `log`? and passing in a mock from this test case? src/test/python/apache/aurora/client/test_config.py (line 252) <https://reviews.apache.org/r/41428/#comment170764> This test is not doing what it claims to do. It's verifying that we raise an AttributeError if an unexpected value is passed in (foo), not that things fail if no command is specified. Pystachio does support type checking for required fields, but it has to be explicitly invoked (i.e. you can construct a `ShellHealthChecker` without passing in a command, but if you were to call `check()` on it, it would return with a TypeCheck error. I'm not super familiar with this part of the codebase, but it looks like maybe we do this when we load the config (though I'm not 100% sure). I'd try loading a config as the above tests do, with `health_checker = ShellHealthChecker()` and see what happens, hopefully that will fail with an invalid config due to missing command line, and we can assert on that failure? src/test/python/apache/aurora/client/test_config.py (line 272) <https://reviews.apache.org/r/41428/#comment170769> Drop this comment as well. - Joshua Cohen On Dec. 16, 2015, 7:08 p.m., Dmitriy Shirchenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41428/ > ----------------------------------------------------------- > > (Updated Dec. 16, 2015, 7:08 p.m.) > > > Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner. > > > Bugs: AURORA-1562 > https://issues.apache.org/jira/browse/AURORA-1562 > > > Repository: aurora > > > Description > ------- > > Refactoring HealthCheckConfig into separate structs > > > Diffs > ----- > > docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f > src/main/python/apache/aurora/client/config.py > 161c3627db0afa8fdd8afff5abf6a94556f53180 > src/main/python/apache/aurora/config/schema/base.py > e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 > src/main/python/apache/aurora/executor/common/health_checker.py > cba4e8c879fae2b9f56a36bf8fa4b702163667d1 > src/test/python/apache/aurora/client/test_config.py > 8fd112fd8a10120f57324d8efec22a009b93404b > src/test/python/apache/aurora/executor/common/test_health_checker.py > 8561abc3e7df8803785b70064bb76553d3210399 > src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora > 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 > > Diff: https://reviews.apache.org/r/41428/diff/ > > > Testing > ------- > > Added unit tests. > Changed end to end test. > > > Thanks, > > Dmitriy Shirchenko > >
