Mcd startup cleanup #862
Mcd startup cleanup #862
Conversation
Prep for further changes.
The startup code was trying to do "use kubeconfig if available" which doesn't make sense to me. As far as I can tell the openshift-ansible code doesn't provide one. Drop it in prep for further changes.
openshift-ci-robot
added
the
size/M
openshift-ci-robot
added
the
approved
|
For the second commit, as long as it doesn't break the Ingnition path, we're good as they only use that for now. |
|
/test e2e-rhel-scaleup This would probably fail on CRI-O anyway |
|
@cgwalters: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
That doesn't seem to break MCD once-from, however we can't fully validate the nodes join until openshift/openshift-ansible#11698 lands |
Previously the `Run` method dispatched on `onceFrom`, it just flows better if we make that an explicitly different path in the start code.
Rather than just doing it in the cluster case.
Since the signal handling is specific to the cluster case, move that bit into `Run`.
openshift-ci-robot
added
size/L
|
Pushed a few more changes to this one. |
|
This is looking good so far!! ❤️ |
Doesn't make sense to do this in `start()` and pass it through the ctors, we only get one value.
Since we now have a `RunOnceFrom`, move the once-from specific args there.
|
Ohh now I see...I'm basically trying to undo some of the changes in 165f8d4 that factored out the boot id into start so the unit tests could pass a dummy value. I think testing most of the MCD is just hard to mock fundamentally. @runcom (and others) how about for the MCD we just unit test "easy" parts and leave more heavy lifting testing to the cluster e2e tests? |
Yeah I agree.. I think the unit tests work best when we are testing some of our logic heavy parts (for ex reconcilable) & "give it this/get that" type functions but don't map that well onto other things imo - sometimes it's either too hard or I end up thinking "I wrote this long unit test but is it really adding any value???" . So I'd say if you feel like the unit test adds value sure, but if you're just doing it to have a unit test but feel like it's diminishing returns it's prob better suited as e2e. |
|
And my bootID changed passed CI because it runs on Linux, but I think Antonio at least was running the unit tests on a Mac, so |
|
Another variant of this is #760 |
|
I think I suggested this earlier but maybe what we can do is call (To emphasize the reason I'm trying to clean up the MCD entrypoint is I need to add a new variant and there was already a lot of duplication with just 2, adding 3 needs cleanup) |
yup, I concur with that. I also think our code should be simple enough and well factored that unit tests are still a thing. Unit and e2e testing is anyway really different and there's value in both (otherwise we would have one or the other). The value in writing units is about writing simple code cause you're forced to factor out functions into simple ones to unit test (and validate them). The MCD is huge thought so again, I agree with that above in general. This is ready to go now right? I can see all tests passing. /approve |
Oh of course. For example I think the tests I added for kargs worked out well; it's just changing data structures in memory, totally safe and appropriate for a unit. Looking at say the ssh key unit tests we mock out the actual writing. But if during a code refactor we stopped using the mocking accidentally the unit tests would try to rewrite the user's ssh keys potentially...
Yeah, but it will break running the unit tests on MacOS as is. Do you care? Any opinion on which approach to take from above? I think I'd lean towards the I can do that as a followup? Or do we want to do it in this PR? |
I'm the only one with MacOS so I'll just go ahead and live with that test till we follow up to fix this with, uhm, yeah /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, runcom The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
7 participants
More prep for #798
I think the second commit is right but need a BYOH team member to verify.