Make Provision.Script a pointer

[vax-r]

Summary

As the TODO in limayaml/defaults.go claims, change the type of
"Script" under "type Provision struct" from "string" to "*string".

Update all callers to pass a pointer and handle "nil" in comparisons,
validation, and marshalling.

[AkihiroSuda]

Thanks

[vax-r]

@AkihiroSuda -
Thanks !
I just update the PR and I think all the CI tests are passed.
Please take a look if you are available !

[jandubois]

I think FillDefaults() should have:

if y.Provision.Script == nil {
    y.Provision.Script = ptr.Of("")
}

instead of having p.Script != nil && *p.Script != "" multiple times in Validate().

---

I would also suggest to change Probe.Script to a pointer as well, for symmetry. It is confusing to see *p.Script in the Provision code, but p.Script in the Probe code for the same thing.

[vax-r]

I think FillDefaults() should have:

if y.Provision.Script == nil {
    y.Provision.Script = ptr.Of("")
}

instead of having p.Script != nil && *p.Script != "" multiple times in Validate().

I would also suggest to change Probe.Script to a pointer as well, for symmetry. It is confusing to see *p.Script in the Provision code, but p.Script in the Probe code for the same thing.

@jandubois -
Thanks for your suggestion .
I'll work on that and send another PR ASAP.

Though I have a question, conventions like p.Script != nil && *p.Script != "" appears multiple places e.g.

lima/pkg/driver/vz/vz_driver_darwin.go

Line 224 in 3819d5b

if cfg.MountType != nil && *cfg.MountType == limatype.NINEP {

Tbh if <variable> == nil and <variable> != nil costs the same instruction but <variable> = ptr.Of("") will incur one more assignment, and this thing will still bump into the check of <variable> != "".
So basically this way will incur one more assignment. I am not sure if it's worth it ?

I would also suggest to change Probe.Script to a pointer as well, for symmetry. It is confusing to see *p.Script in the Provision code, but p.Script in the Probe code for the same thing.

I get your idea on this part, just have question for the former part.
Thanks.

[jandubois]

So basically this way will incur one more assignment. I am not sure if it's worth it ?

I'm not concerned about the "cost" at all; it is trivial. I just want to simplify the code for the parts where having a value is not really optional anyways.

With the exception of the Ansible provisioning mode, not having a script will result in an error during validation. So we could just as well keep the rest of the validation code simpler.

It is also more maintenance friendly, as future changes cannot "forget" to check for nil because once the template has defaults filled in, there should be no more nils for non-optional data.

In my mind, only struct pointers should be allowed to have nil values, but pointers to string, bool, or int should be set to a pointer of their respective null value.

[vax-r]

In my mind, only struct pointers should be allowed to have nil values, but pointers to string, bool, or int should be set to a pointer of their respective null value.

Thanks for your explanation ! makes sense to me, I'll ammend them up ASAP.