akutz / gnotify

0 stars 1 forks source link

Another issue with a long body #3

Closed akutz closed 11 months ago

akutz commented 11 months ago

What does this PR do, and why is it needed?

Per a conversation with @bryanv, 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.

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 NA

Are 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 NONE since v1alpha2 is not yet released.

NONE
akutz commented 11 months ago

Cool comment!

akutz commented 11 months ago

Another cool comment!