-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
v33tov32: fix 3.3 -> 3.2 Clevis pointer translation #47
Conversation
Thank you for doing this, and I take a look and see what all the fuss is about. |
0ab6830
to
342d8e5
Compare
Added a basic |
I am currently pondering how to move this change to be more of an after the fact effect like the rest of the checks. TBC this change does work, and catches / fixes the issue. However, typically that section is really a copy and paste from the ignition repo and I would like to keep that true, though I am not sure what to do about that. @bgilbert I wonder if you have any insight? |
Thanks for doing this! Do we need a similar change for
The
I don't see a straightforward way to do that, and wouldn't worry about it. The converter code doesn't need to be super-clean, and the switch from pointers to embedded structs is a one-off change that won't happen again. (Ignition now has unit tests to make sure we don't get more struct pointers in the future.) Let's just make sure we update the leading comment block to indicate what changes were made to the original translation code. |
244641b
to
81c8164
Compare
@bgilbert I updated the comment to include some details about the change that breaks the pattern, and we don't need to do anything with the link.target, it looks like the other direction was accounted for previously. |
7b939cc
to
557113f
Compare
If we populate the "Clevis" section of ignition in 3.3 and then try to down-translate to 3.2, we panic because some of the fields were pointers in 3.2, but they are not pointers in 3.3 (so you copy from a struct in 3.3 into a nil pointer in 3.2 and you die) For Clevis and Custom, this just compares the struct to the empty struct, and: - if it's equal, we leave the pointer as a nil and skip the translate - if it's not equal, we point the pointer at an empty struct and do the translation into that co-authored-by: jkyros jkyros@redhat.com
557113f
to
9e30128
Compare
Per openshift/machine-config-operator#3576 (comment),
Clevis
was a pointer in 3.2, but isn't in 3.3, so down-translation from 3.3 -> 3.2 resulted in a panic.I also added a test here, but I can retool it or drop it.
I was going to add a test for
Clevis.Custom
also, but it looks likepin
is simultaneously required and and an error if supplied, so I gave up? I'm probably missing...something 😄 :https://github.com/coreos/ignition/blob/0680c483fce356c3ddd2bcec913f41e847adc51e/config/v3_2/types/custom.go#L28
https://github.com/coreos/ignition/blob/0680c483fce356c3ddd2bcec913f41e847adc51e/config/v3_3/types/clevis.go#L36
/cc @prestist