> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/client/config.py, lines 111-113
> > <https://reviews.apache.org/r/41428/diff/4/?file=1166713#file1166713line111>
> >
> >     Nit, could probably inline all of this:
> >     
> >         if not health_check_end_config.get(SHELL_HEALTH_CHECK, 
> > {}).get('shell_command'):
> >           die(...)

I personally prefer not to chain more than 2 operations on one line as it makes 
it harder for me to step through variables when debugging. So, if I may, I will 
respectfully decline this suggestion.


> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/client/test_config.py, lines 240-243
> > <https://reviews.apache.org/r/41428/diff/4/?file=1166716#file1166716line240>
> >
> >     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?

I personally use mock.patch decorators myself but I try not to add dependencies 
as well so I had to figure out how to make this work with pytest. I feel like 
mock.patch is the way to go but I also feel like we should be consistent and 
swapping to mock is a separate chunk of work as it may be required affect other 
tests as well. Like it would be weird to just have one test using mock.

In any case: is this a nit or you feel like this is a blocker?


> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/client/test_config.py, line 187
> > <https://reviews.apache.org/r/41428/diff/4/?file=1166716#file1166716line187>
> >
> >     Kill this comment? It seems superfluous.

I added because I had a hard time finding my tests. I like comments to 
logically separate tests into sections. If you feel like they should be 
separated into individual classes, then I can do that as well but comments 
seemed good enough for tests. Lmk.


> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/client/test_config.py, line 276
> > <https://reviews.apache.org/r/41428/diff/4/?file=1166716#file1166716line276>
> >
> >     Drop this comment as well.

Same response as above. ^


> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/client/config.py, line 111
> > <https://reviews.apache.org/r/41428/diff/4/?file=1166713#file1166713line111>
> >
> >     Try to avoid abbreviations in variable names. Just call this 
> > `shell_health_checker`?

Done.


> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/config/schema/base.py, line 59
> > <https://reviews.apache.org/r/41428/diff/4/?file=1166714#file1166714line59>
> >
> >     We should also rename `endpoint_config` to `health_checker`.
> >     
> >     And probably rename the above `DefaultEndpointConfig` to 
> > `DefaultHealthChecker`

Done.


> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/client/test_config.py, line 256
> > <https://reviews.apache.org/r/41428/diff/4/?file=1166716#file1166716line256>
> >
> >     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?

Done.


- Dmitriy


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41428/#review110741
-----------------------------------------------------------


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