Add helper functions to work with Ignition Configs #2870
Conversation
mkenigs
force-pushed
the
helpers
branch
2 times, most recently
from
5c4ae4c to
3dc07fc
Compare
pkg/controller/common/helpers.go
Outdated
There was a problem hiding this comment.
@sinnykumari @jkyros I think the conclusion from the thread on John's PR was to leave the debug statements in. Are we wanting to keep or remove the comment?
There was a problem hiding this comment.
I think I'd vote for having the caller log the diff as a whole, which could be done more easily with the above change of returning a struct.
There was a problem hiding this comment.
Returning struct with diff would be much better.
These functions will be needed by both openshift#2802 and openshift#2851 so adding them here to avoid merge conflicts later Moved/renamed newFile -> NewIgnFile
|
|
||
| // CalculateConfigFileDiffs compares the files present in two ignition configurations and returns the list of files | ||
| // that are different between them | ||
| func CalculateConfigFileDiffs(oldIgnConfig, newIgnConfig *ign3types.Config) []string { |
There was a problem hiding this comment.
WDYT about returning a struct type with the diff that distinguishes add/remove/modify?
There was a problem hiding this comment.
Do we need that info somewhere in MCO?
There was a problem hiding this comment.
We could use it for isDrainRequired (see #2851)
There was a problem hiding this comment.
This can be at least useful to log messages by caller instead of doing it here,in the context of https://github.com/openshift/machine-config-operator/pull/2870/files#r768161851
There was a problem hiding this comment.
@jkyros wdyt? Time to pull out the howitzer or not?
There was a problem hiding this comment.
I think given this is needed for #2851 which I think we want to get in before Friday, it might be better to fix this later if we decide it's worth it
There was a problem hiding this comment.
I think we end up having to someday, but that day doesn't necessarily have to be today if there are timeline concerns.
- Any half-measure we take is just going to end up here again later
- This would let us keep our debug logging without forcing it on all function consumers.
- I don't know how much of the utility here you need/want for Don't perform registries.conf checks on GPG change #2851 ?
- I do think I can rebase in time on Send alert when MCO can't safely apply updated Kubelet CA on nodes in paused pool #2802 if that was a concern
There was a problem hiding this comment.
sure, let's create a card for this in Jira so that someone pick this up later on.
pkg/controller/common/helpers.go
Outdated
There was a problem hiding this comment.
I think I'd vote for having the caller log the diff as a whole, which could be done more easily with the above change of returning a struct.
|
@mkenigs: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
sinnykumari
left a comment
sinnykumari
left a comment
There was a problem hiding this comment.
LGTM
Will leave this for you in case you want to fix https://github.com/openshift/machine-config-operator/pull/2870/files#r768161532
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mkenigs, sinnykumari 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 |
openshift-ci
bot
added
the
approved
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
3 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/skip |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
If a config change does not contain changes to registries.conf, don't apply checks specific to registries.conf Also start using helper functions added in openshift#2870
7 participants
These functions will be needed by both
#2802
and
#2851
so adding them here to avoid merge conflicts later
Moved/renamed newFile -> NewIgnFile