search

crossplane-contrib / provider-jet-azure

Apache License 2.0
17 stars 20 forks source link

ResourceGroupName should be required #76

Open vfarcic opened 2 years ago

vfarcic commented 2 years ago

It is not clear from https://doc.crds.dev/github.com/crossplane-contrib/provider-tf-azure/kubernetes.azure.tf.crossplane.io/KubernetesCluster/v1alpha1@v0.2.2 that resourceGroupName, or resourceGroupNameRef, or resourceGroupNameSelector are mandatory (one of the three).

ulucinar commented 2 years ago

Hi @vfarcic, Thank for reporting this. We had a discussion on referencer field markers with @muvaf and our conclusion back then was something similar to the following considering some of the discussion in https://github.com/crossplane/crossplane/blob/master/design/one-pager-managed-resource-api-design.md:

// +immutable
ResourceGroupName string `json:"resourceGroupName,omitempty"`
// +optional
// +immutable
ResourceGroupNameRef *xpv1.Reference `json:"resourceGroupNameRef,omitempty"`
// +optional
ResourceGroupNameSelector *xpv1.Selector `json:"resourceGroupNameSelector,omitempty"`

Right now, what we are generating with Terrajet is:

// +kubebuilder:validation:Optional
ResourceGroupName *string `json:"resourceGroupName,omitempty" tf:"resource_group_name,omitempty"`

// +kubebuilder:validation:Optional
ResourceGroupNameRef *v1.Reference `json:"resourceGroupNameRef,omitempty" tf:"-"`

// +kubebuilder:validation:Optional
ResourceGroupNameSelector *v1.Selector `json:"resourceGroupNameSelector,omitempty" tf:"-"`

Btw, we can generalize this issue for all cross-resource referencer fields, and probably discuss this in API design doc.

@muvaf, what do you think? Should we update the markers we generate in Terrajet?

ulucinar commented 2 years ago

Regardless of the markers, because we have no marker defining a set of fields, we had also better to link this to: https://github.com/crossplane-contrib/terrajet/issues/140

One alternative could be just generating the referencer fields (specified with the +crossplane:generate:reference markers) with more informative comments.

muvaf commented 2 years ago

@turkenh do you think if it'd be possible to add a one-liner before the referencer marker in the description of the field to say at least one of the three needs to be given if the field is required originally?