Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Settle down on the naming condition: somethingRef vs somethingName #91

Closed
dtantsur opened this issue Nov 27, 2024 · 8 comments · Fixed by #92
Closed

Settle down on the naming condition: somethingRef vs somethingName #91

dtantsur opened this issue Nov 27, 2024 · 8 comments · Fixed by #92
Assignees
Labels
triage/accepted Indicates an issue is ready to be actively worked on.
Milestone

Comments

@dtantsur
Copy link
Member

dtantsur commented Nov 27, 2024

I've read somewhere (forgot where) that using SomethingRef of type corev1.LocalObjectReference is the preferred way to link to other objects.

On one hand, it allows adding Kind/Namespace later if needed without a breaking change.

On the other, it feels ugly, and does not seem to be used consistently in other major operators. Postgres uses SecretName/CaSecretName for TLS. CAPI operator has AdditionalManifestsRef. BMO is inconsistent: ConsumerRef, NetworkData, PreprovisioningNetworkDataName.

We need to decide before the MVP.

/triage accepted

@dtantsur dtantsur added this to the IrSO MVP milestone Nov 27, 2024
@dtantsur dtantsur self-assigned this Nov 27, 2024
@metal3-io-bot metal3-io-bot added the triage/accepted Indicates an issue is ready to be actively worked on. label Nov 27, 2024
@dtantsur
Copy link
Member Author

/cc @tuminoid @lentzi90 @elfosardo @Rozzii

Looking for feedback here.

@tuminoid
Copy link
Member

Ref suffix is more flexible for future changes, or when it refers across namespaces, but when relationship is within same namespace and of known type, Name seems to be used.

I quickly grepped the repos I have cloned (opinionated sample of course) for hits on Name: and Ref: in manifests and its roughly split 50/50, even within projects. Almost all repos have both.

Lets also ping @adilGhaffarDev and @kashifest for this.

@dtantsur
Copy link
Member Author

Similarly, I'm starting to think that we need simple names for links to IronicDatabase, TLS and credentials secrets, and so on.

dtantsur added a commit to dtantsur/ironic-standalone-operator that referenced this issue Nov 28, 2024
As discussed in
metal3-io#91, lets
use simpler reference names and reserve the SomethingRef format for when
the field can refer to different kinds or at least namespaces.

This is quite a breaking change but I don't see a lot of value in going
through deprecation since it's going to significantly complicate the
code nobody should be using in production yet.

Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
@kashifest
Copy link
Member

Not an expert in this regard but I am leaning towards Refs (although I don't have any strong points against names.)

@dtantsur
Copy link
Member Author

Hmm, we got a split here. I've just posted a WIP patch using names, so that you can compare two approaches.

dtantsur added a commit to dtantsur/ironic-standalone-operator that referenced this issue Nov 28, 2024
As discussed in
metal3-io#91, lets
use simpler reference names and reserve the SomethingRef format for when
the field can refer to different kinds or at least namespaces.

This is quite a breaking change but I don't see a lot of value in going
through deprecation since it's going to significantly complicate the
code nobody should be using in production yet.

Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
@adilGhaffarDev
Copy link
Member

To my understanding, ObjectReference is used when it is not clearwhat will be in the field and depends on the outside entity. For example in CAPI ObjectReference is mostly used when something that is being set to a field is coming from outside. like here:
https://github.com/kubernetes-sigs/cluster-api/blob/2719ae3cc94bbd72dbcbfecc254615487693de86/api/v1beta1/machinehealthcheck_types.go#L110
RemediationTemplate is provided by the infrastructure provider.
In these kinds of scenarios ObjectReference is better. The same explanation goes for ConsumerRef in BMO.

Otherwise, I think it's always better to have your own struct types.

@adilGhaffarDev
Copy link
Member

adilGhaffarDev commented Nov 28, 2024

regarding LocalObjectReference, after reading this:

// LocalObjectReference contains enough information to let you locate the
// referenced object inside the same namespace.
// +structType=atomic
type LocalObjectReference struct {
	// Name of the referent.
	// This field is effectively required, but due to backwards compatibility is
	// allowed to be empty. Instances of this type with an empty value here are
	// almost certainly wrong.
	// TODO: Add other useful fields. apiVersion, kind, uid?
	// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
	// +optional
	// +default=""
	// +kubebuilder:default=""
	// TODO: Drop `kubebuilder:default` when controller-gen doesn't need it https://github.com/kubernetes-sigs/kubebuilder/issues/3896.
	Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"`
}

it's better to have your own struct type and not depend on LocalObjectReference.

dtantsur added a commit to dtantsur/ironic-standalone-operator that referenced this issue Dec 2, 2024
As discussed in
metal3-io#91, lets
use simpler reference names and reserve the SomethingRef format for when
the field can refer to different kinds or at least namespaces.

This is quite a breaking change but I don't see a lot of value in going
through deprecation since it's going to significantly complicate the
code nobody should be using in production yet.

Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
@elfosardo
Copy link
Member

bit late to the party here but I think the renaming makes sense in terms of semplification and clarity
I've LGTMed Dmitry's patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/accepted Indicates an issue is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants