Ignition types.Config (v0s2) to runtime.RawExtension conversion #996
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
|
/retest |
1 similar comment
|
/retest |
openshift-ci-robot
added
the
needs-rebase
LorbusChris
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
openshift-ci-robot
removed
the
needs-rebase
|
/retest |
1 similar comment
|
/retest |
|
I'm still going through this, but can you break this up into 2 commits and break out the test changes to make it easier to view? |
|
@runcom @LorbusChris Still going through this, but basic question to check my understanding: Currently: Machineconfig.Spec.Config is an Ignition Config. Won't this make troubleshooting/looking at MachineConfigs generally impossible from a user perspective? Am I missing something???? |
|
@kikisdeliveryservice please hold off a thorough review for now, this still needs polishing. With this PR, Spec.Config.Raw is going to be []byte. It's not pretty, but it'll allow us to use Ignition Config spec2 as well as spec3 (by encoding/decoding Spec.Config.Raw from/to either of those). As we discourage writing Ignition by hand anyway, I don't see a huge problem here. What specific troubleshooting steps do you see becoming much more difficult to do with this PR? |
|
RawExtension basically allows an arbitrary struct to be put there and we decide the destination struct to unmarshal to. Iirc this won't change how we write MC or how they're printed right? |
My main question is if a MC has an Ign Config (which people do indeed write) that is encoded as []byte, if someone is inspecting MCs will this PR affect what they see in Spec.Config of the MC? Will it be readable? Are we changing the UI? |
openshift-ci-robot
added
the
needs-rebase
openshift-ci-robot
removed
the
needs-rebase
|
/retest |
|
I'm taking the hold off that I applied for more review earlier. |
openshift-ci-robot
removed
the
do-not-merge/hold
|
@runcom PTAL |
|
would like another review on this as this is a large change and we only have 1 approval/lgtm from same person. @runcom was reviewing earlier so would like him to review/sign off that his comments were addressed and final review is good. /hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
/lgtm |
openshift-ci-robot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LorbusChris, runcom, yuqi-zhang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/refresh |
|
/skip |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
8 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@LorbusChris: 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. Instructions 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. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/test e2e-gcp-op |
With the introduction of rawExtension for Ignition config objects via openshift#996, we also now use ignition.Parse directly to process the validity of ignition configs. This will cause machineconfigs without a Ignition section to fail, which we don't want. Also modify some error messages for clarity. Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
- What I did
Convert Ignition types.Config to runtime.RawExtension
In order to decouple the MCO's API from the Ignition types,
this PR introduces conversion to and from RawExtension.
When reading or mutating a config, the RawExtension is now converted to
a typed Ignition config internally upon receival, and then converted
back to RawExtension for passing it around externally.
Using RawExtension does not change the wire-format of the
transmitted data. The contents of MachineConfig.Spec.Config
will still be in JSON format and readable as such.
Move MergeMachineConfigs func into controller/common pkg,
in preparation of decoupling the API from Ignition Config types.
This PR should fully unblock the migration of the
apipackage to theopenshift/apirepo.I'd advise to review each commit separately.
- How to verify it
CI
- Description for the changelog
Added Ignition types.Config (version 0.x spec 2.x) to runtime.RawExtension conversion