refactor lookup_chronos_jobs #24
Conversation
Got rid of regex, max_expected. Introduce service, instance. All the unit tests pass, which means all callers are being honest and mocking correctly :)
I decided this is going to be better when I handle git_hash and config_hash, which is next.
note1: I decided not to bother with the [de]compose_job_id requirement that you either specify both git_hash and config_hash or you specify neither. I don't thnk this function needs it and it didn't make the implementation any more complicated. note2: Technically my test is insufficient since it doesn't differentiate matching just git_hash vs matching just config_hash. I don't think the extra test granularity adds any value so I'm not going to bother. (I did comment out both sides of the git_hash/config_hash and block to manually verify that both sides work.)
…_chronos_jobs. This is a long-delayed repsonse to kwa and robj comments in review:117855. In my analysis, the underlying theme in their complaints is that lookup_chronos_job is designed wrong. Correcting this malady is what has motivated my last several refactoring branches: making [de]compose_job_id more sensible, reducing the number of different ways to accomplish various common job_id transformations, etc.
| try: | ||
| regexp = re.compile(pattern) | ||
| except re.error: | ||
| raise ValueError("Invalid regex pattern '%s'" % pattern) | ||
| jobs = client.list() |
There was a problem hiding this comment.
I'd be tempted to parameterise the client.list, and rename the function to 'match_chronos_jobs' or something similar. There is a function that in chronos_tools.py called ``match_jobs_names_to_service_instance` that does something similar (can you remove that function as part of this, too?)
There was a problem hiding this comment.
Re: parameterization: I don't think that's desirable since literally every caller of lookup_chronos_jobs would just call client.list() right before calling lookup_chronos_job (I guess chronos_serviceinit.perform_command could get away with calling it just once before the three lookups_chronos_job calls in the big if block). It wouldn't have much of an effect on the shape of test code. So I'm going to leave that part alone.
Re: match_job_names_to_service_instance: Yes, these functions look very similar now. I'll simplify down. Thanks for the pointer.
|
This looks great to me! |
…roved lookup_chronos_jobs
|
r2 - Elide |
This was after discussion with robj in #24 and then in person. Basically this is a compromise between SRP and DI (which favor calculating the list of jobs separately from filtering it) and YAGNI (currently every caller of lookup_chronos_jobs would need to do its own client.list() and pass that in). Hence, filter_chronos_jobs is fully SRP and DI while lookup_chronos_jobs is less pure but more convenient.
|
r3 - lookup_chronos_jobs now calls filter_chronos_jobs. |
|
shipit - I think this looks good. Thanks for persevering. |
This is #24 but I forgot to fix the upstream branch so closing the PR did something silly and didn't update master.
Back in review:117855, both @solarkennedy and @Rob-Johnson expressed displeasure with how get_matching_jobs() was implemented. Rob further indicted lookup_chronos_jobs(). I punted at the time because I wanted to get status working for chronos jobs and the whole workflow needed significant refactoring.
This PR, as well as my last couple, address this area with an eye toward reducing clutter and making the API more consistent.