Prep commits for having the MCD process encapsulated MachineConfig#935
Prep commits for having the MCD process encapsulated MachineConfig#935cgwalters wants to merge 3 commits intoopenshift:masterfrom
Conversation
The `getStateAndConfigs()` function loads a bunch of data, might as well have it load the pending config too rather than passing it in.
Previously `getNodeAnnotation` read the initial annotations file, and `getStateAndConfigs` also checked for its existence again. Now we have a more clearly named `ensureNodeAnnotations` method that's close to the other code handling the early state, and it returns a `bool` denoting whether or not we found the initial annotations file that's passed down into `getStateAndConfigs`.
So that function is now entirely about the config annotations, and this extra random bit is outside of it. And move down part of the comment to clarify the flow.
openshift-ci-robot
added
size/L
cgwalters
changed the title
| d, err := ioutil.ReadFile(constants.InitialNodeAnnotationsFilePath) | ||
| if err != nil && !os.IsNotExist(err) { | ||
| return false, fmt.Errorf("failed to read initial annotations from %q: %v", constants.InitialNodeAnnotationsFilePath, err) | ||
| } |
There was a problem hiding this comment.
super silly nit: shouldn't we return true from now on here? I'm just worried in the future we're gonna change the outer logic and might miss the bootstrap bool
There was a problem hiding this comment.
Would that would apply for the json.Unmarshal and setNodeAnnotations(..., initial) error cases as well?
There was a problem hiding this comment.
yes, from this point it should be safe to return true
|
/approve |
|
[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 |
There was a problem hiding this comment.
I'm still reluctant to remove a test however easy and straightforward it is...if someone messes up with the skipReboot if this was going to catch it
There was a problem hiding this comment.
We talked about these unit tests before right?
There was a problem hiding this comment.
yup, no blocking, was just pointing out my initial reaction when a test gets deleted
There was a problem hiding this comment.
note for next time (don't have to fix in this pr): can remove emptyMC declaration and just do oldConfig = &mcfgv1.MachineConfig{} directly
There was a problem hiding this comment.
why don't we update the cached copy dn.node = n like in the other case?
openshift-ci-robot
added
the
needs-rebase
|
@cgwalters: PR needs rebase. 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. |
There was a problem hiding this comment.
Just wondering, since this function is not utilizing Daemon struct anymore, is there a reason to keep it part of Daemon ?
There was a problem hiding this comment.
not really, we can remove the object reference to Daemon as a cleanup
|
@cgwalters there are some conflict with latest master, can we get this PR rebased? |
cgwalters
force-pushed
the
mcd-encap-mcd-prep
branch
from
9023e28 to
72cb43d
Compare
|
@cgwalters: The following tests 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. |
|
Eh, the reboot parts of this were merged, and there's been a lot of code churn since then that makes rebasing this tricky/painful. I'm just going to try putting everything in #904 instead. |
6 participants
Prep commits for #904