refactor: split Reconciler from Reconcile in a few methods #926
Conversation
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@fgalind1 Thanks for your contribution and sorry for the delay! We're planning to review and merge this BEFORE our next minor release(=a feature release). The rationale is that, although I believe you've done it perfectly, I'm not sure if this doesn't actually break anything, as our unit test and integration test suite are far from 100% coverage. So I'd like to make it as a part of a feature release rather than a patch release, so that we can better communicate via the semver that it may break something(unexpectedly) due to new features) than nothing. |
fgalind1
force-pushed
the
refactor-controller
branch
from
d90b776
to
00e52ca
Compare
|
Maybe you introduced some compile errors when you kindly tried to rebase your work on our latest main branch! I'll try to fix it myself. Wish I'll be able to manage it! |
Reduce a bit the size of Reconcile() method from ~400 to ~300 to be easier to understand
mumoshu
force-pushed
the
refactor-controller
branch
from
00e52ca
to
94e58c7
Compare
| } else if offline { | ||
| if registrationOnly { | ||
| log.Info( | ||
| "Observed that registration-only runner for scaling-from-zero has successfully been registered.", | ||
| "podCreationTimestamp", pod.CreationTimestamp, | ||
| "currentTime", currentTime, | ||
| "configuredRegistrationTimeout", registrationTimeout, | ||
| ) | ||
| } else if registrationDidTimeout { |
There was a problem hiding this comment.
Seems this is duplicate of L327-L334
There was a problem hiding this comment.
Hey! I was able to fix the build error and I manually took diffs around these changes:
- Extraction of
processRunnerDeletion - Extraction of
processRunnerCreation - Return on
r.processRunnerCreationso that you can decrease the indent-level of all the lines bellow that return (formerly it was intended within aelseblock)
Everything looks good now.
If there's next chance you kindly contribute to this project, if I could ask, it would be super helpful if you could split it into 3 commits like the above so that it is immediately clear for maintainers where to look for review!
Thank you for your contribution. This LGTM now. Merging.
Will do, thanks on this.
:) |
Reduce a bit the size of
Reconcile()method from ~400 to ~300 to be easier to understandFunctionality kept the same, just some logic from
Reconcilewas abstracted into 3 additional methods