This is an automatically generated e-mail. To reply, visit:

src/main/python/apache/aurora/client/config.py (line 103)

    Try to avoid abbreviations in variable names. Just call this 

src/main/python/apache/aurora/client/config.py (lines 103 - 105)

    Nit, could probably inline all of this:
        if not health_check_end_config.get(SHELL_HEALTH_CHECK, 

src/main/python/apache/aurora/config/schema/base.py (line 59)

    We should also rename `endpoint_config` to `health_checker`.
    And probably rename the above `DefaultEndpointConfig` to 

src/test/python/apache/aurora/client/test_config.py (line 187)

    Kill this comment? It seems superfluous.

src/test/python/apache/aurora/client/test_config.py (lines 236 - 239)

    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)

    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% 
    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)

    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

Reply via email to