⚠️ Change v1a2 VM spec fields of type struct to ptrs #279
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
testing-needed-e2e-fast
size/XL
bryanv
approved these changes
Nov 30, 2023
akutz
force-pushed
the
feature/vm-spec-w-nil-fields
branch
2 times, most recently
from
529e8be to
b6f0ee5
Compare
This patch updates the fields in v1alpha2/VirtualMachineSpec type such
that if the field's type is a struct, where type is T, the field is now
of type *T.
This change is to prevent user confusion. Otherwise, when printing a
VM with kubectl, the YAML may resemble the following:
apiVersion: vmoperator.vmware.com/v1alpha2
kind: VirtualMachine
metadata:
name: my-vm
namespace: my-namespace
spec:
className: my-vm-class
imageName: my-vm-image
storageClass: my-storage-class
advanced: {}
bootstrap: {}
network: {}
reserved: {}
Instead of the user seeing the empty objects, this change will result in
the following output:
apiVersion: vmoperator.vmware.com/v1alpha2
kind: VirtualMachine
metadata:
name: my-vm
namespace: my-namespace
spec:
className: my-vm-class
imageName: my-vm-image
storageClass: my-storage-class
The reason the empty objects were there in the first place was due to
the way the diffs are calculated with mutation webhooks. Because Go's
JSON library is used to calculate these diffs, even if the user did not
submit a value for the field, the JSONPatch will set the field to an
empty object.
For what it is worth, the reason for the initial design is because it is
best practice in Golang to avoid pointers whenever possible. Go's memory
management is much more efficient when pointers are not involved. It is
also the case that non-pointer fields remove the possibility of NPEs,
not to mention simplify nil-value checks throughout the code. However,
the goal of VM Operator is to make the best *user* experience possible.
Therefore, while this patch increases code complexity and introduces a
number of boilerplate nil-checks/assignments, these concerns are
outweighed by the value added to the end-user experience.
akutz
force-pushed
the
feature/vm-spec-w-nil-fields
branch
from
b6f0ee5 to
b544654
Compare
Minimum allowed line rate is |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do, and why is it needed?
Per a conversation with @bryanv, this patch updates the fields in
v1alpha2/VirtualMachineSpectype such that if the field's type is a struct, where type isT, the field is now of type*T.This change is to prevent user confusion. Otherwise, when printing a VM with kubectl, the YAML may resemble the following:
Instead of the user seeing the empty objects, this change will result in the following output:
The reason the empty objects were there in the first place was due to the way the diffs are calculated with mutation webhooks. Because Go's JSON library is used to calculate these diffs, even if the user did not submit a value for the field, the JSONPatch will set the field to an empty object.
Which issue(s) is/are addressed by this PR? (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes
NAAre there any special notes for your reviewer:
For what it is worth, the reason for the initial design is because it is best practice in Golang to avoid pointers whenever possible. Go's memory management is much more efficient when pointers are not involved. It is also the case that non-pointer fields remove the possibility of NPEs, not to mention simplify nil-value checks throughout the code. However, the goal of VM Operator is to make the best user experience possible. Therefore, while this patch increases code complexity and introduces a number of boilerplate nil-checks/assignments, these concerns are outweighed by the value added to the end-user experience.
Please add a release note if necessary:
Please note the release note is
NONEsince v1alpha2 is not yet released.