Bug 1904663: Fix comparison in pointer ignition customization asset #4455
Conversation
|
This fixes the issue mentioned in the comments of #4413 by @cgwalters |
|
/cc @kirankt |
hardys
force-pushed
the
pointer_mc_fix
branch
from
928599e to
1308fc7
Compare
| if savedPointerIgnition != defaultPointerIgnition { | ||
| savedPointerIgnitionJson, err := ignition.Marshal(savedPointerIgnition) | ||
| defaultPointerIgnitionJson, err := ignition.Marshal(defaultPointerIgnition) | ||
| if string(savedPointerIgnitionJson) != string(defaultPointerIgnitionJson) { |
There was a problem hiding this comment.
Maybe reflect.DeepEqual instead? Not sure if there's much distinction, but DeepEqual is used elsewhere in the installer for e.g. comparing on disk and in-memory assets.
There was a problem hiding this comment.
When I used the reflect.DeepEqual, it returned false consistently in testing. This method from @hardys looks to be more fool-proof, IMHO,
There was a problem hiding this comment.
Yeah I did consider DeepEqual but decided to just use the exact same format the underlying asset does, e.g the ignition configs get serialized via ignition.Marshal, and the thing we ultimately care about after loading them is did that json data change, so a string comparison seemed like the simpler and more consistent option.
There was a problem hiding this comment.
Right, upon reflection, they're equal approaches.
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: staebler 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 |
openshift-ci-robot
added
the
approved
|
/retitle Bug 1904663: Fix comparison in pointer ignition customization asset |
openshift-ci-robot
changed the title
openshift-ci-robot
added
the
bugzilla/severity-high
|
@hardys: This pull request references Bugzilla bug 1904663, which is valid. 3 validation(s) were run on this bug
In response to this:
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. |
openshift-ci-robot
added
the
bugzilla/valid-bug
hardys
force-pushed
the
pointer_mc_fix
branch
2 times, most recently
from
2b36b5a to
fa8f4e5
Compare
An issue was spotted after openshift#4413 landed, the config comparison always evaluates to true, so we're generating empty machineconfig objects even when no customization has happened. Update to compare a json representation of the configs, and add a test to confirm all works as expected.
hardys
force-pushed
the
pointer_mc_fix
branch
from
fa8f4e5 to
2da500a
Compare
|
/lgtm |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@hardys: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
|
@hardys: All pull requests linked via external trackers have merged: Bugzilla bug 1904663 has been moved to the MODIFIED state. In response to this:
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. |
An issue was spotted after #4413 landed, the config comparison
always evaluates to true, so we're generating empty machineconfig
objects even when no customization has happened.
Update to compare a json representation of the configs, and add
a test to confirm all works as expected.