using pointers for optional structs

[mikebz]

Problem:

We now have open sourced ConfigSync and we have pkg/api which assumes that some people might import that struct definition and use them to create ConfigSync yaml output. This is what I am doing in a prototype for generation of multi-cluster platform repo.

The way the secrets used to be declared (and there are other structs like that) actually didn't make them nullable. Go yaml generator would always output a {} because the structs were not actually null.

	// secretRef is the secret used to connect to the Git source of truth.
	// +nullable
	// +optional
	SecretRef SecretReference `json:"secretRef,omitempty"`

This PR is part 1 of 2 where the secrets are now declared as pointers

	// secretRef is the secret used to connect to the Git source of truth.
	// +nullable
	// +optional
	SecretRef *SecretReference `json:"secretRef,omitempty"`

Potential danger:

• If someone has imported the previous version of this go struct we will break them, but the fix is easy.
• potentially there is a case I missed where we are setting the name of the struct but I missed the struct creation code. Alternatively the getting now has to account for nil.
• I have created a helper function to deal with this situation, but it does cross-pollinate v1beta1 and v1alpha1, if we really want to keep them separate, we can move this function into another package.

Testing

• Locally make test worked
• CI e2e tests should pass too before we merge.

[mikebz]

@janetkuo not sure what the protocol is for resolving comments - if the sender or receiver resolves them. Leaving them open for now.

[janetkuo]

not sure what the protocol is for resolving comments - if the sender or receiver resolves them. Leaving them open for now.

It's fine for PR author to resolve a comment if the change to resolve the comment is trivial, or no further discussion is needed for that comment.

[sdowell]

The change looks good. Need to regenerate CRDs after API changes. make configsync-crds should do the trick.

@sdowell did we document this in the contributor guide?

No I don't believe this is documented in the contribution guide but agreed that we should add it. We should also add a presubmit check around this IMO

[mikebz]

The change looks good. Need to regenerate CRDs after API changes. make configsync-crds should do the trick.

I was hoping that this would not have any effect on the outside world, but I see that the "nullable" does have an effect. I don't think this is going to break any client, but I am not sure - I haven't pushed any CRD based APIs out into production yet. Is that an innocuous change?

[mikebz]

/label tide/merge-method-squash

[mikebz]

/test kpt-config-sync-presubmit-e2e-mono-repo-test-group2

[janetkuo]

I was hoping that this would not have any effect on the outside world, but I see that the "nullable" does have an effect. I don't think this is going to break any client, but I am not sure - I haven't pushed any CRD based APIs out into production yet. Is that an innocuous change?

Confirmed with @apelisse it's only breaking for people using the go client (typed client). Users who install Config Sync wouldn't be impacted. The breakage is listed as potential danger in your PR description, and it's easy to fix.

[google-oss-prow[bot]]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: janetkuo

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:

• ~~OWNERS~~ [janetkuo]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment