Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Fix type collisions in the same CRD package #24

Merged
merged 2 commits into from
Aug 23, 2021
Merged

Conversation

muvaf
Copy link
Member

@muvaf muvaf commented Aug 20, 2021

This PR adds order for the iteration by sorting the keys and produce every type it encounters. If there is a name collision we add the former field name until we find a unique name, which is guaranteed. In the above example, we'd have one TimeoutParameters type and the second one would have TcpRouteTimeoutParameters name. Since we sort, always the same type will get to have a prefix.

Fixes #23

…same CRD package

Signed-off-by: Muvaffak Onus <me@muvaf.com>
Signed-off-by: Muvaffak Onus <me@muvaf.com>
@turkenh
Copy link
Member

turkenh commented Aug 23, 2021

@muvaf just gave this a try and noticed that it is working as you described.

But I am wondering if there could be a way to detect and reuse same structs instead of duplication with exact same fields.

For example, checking apis/emr/v1alpha1/emrcluster/zz_types.go generated with this and we have a couple of EbsConfigParameters which are exactly identical. Couldn't we somehow figure that out and reuse it if this is the case?

type EbsConfigParameters struct {
	Iops *int64 `json:"iops,omitempty" tf:"iops"`

	Size int64 `json:"size" tf:"size"`

	Type string `json:"type" tf:"type"`

	VolumesPerInstance *int64 `json:"volumesPerInstance,omitempty" tf:"volumes_per_instance"`
}

...

type MasterInstanceGroupEbsConfigParameters struct {
	Iops *int64 `json:"iops,omitempty" tf:"iops"`

	Size int64 `json:"size" tf:"size"`

	Type string `json:"type" tf:"type"`

	VolumesPerInstance *int64 `json:"volumesPerInstance,omitempty" tf:"volumes_per_instance"`
}

...

type CoreInstanceGroupEbsConfigParameters struct {
	Iops *int64 `json:"iops,omitempty" tf:"iops"`

	Size int64 `json:"size" tf:"size"`

	Type string `json:"type" tf:"type"`

	VolumesPerInstance *int64 `json:"volumesPerInstance,omitempty" tf:"volumes_per_instance"`
}

...

type InstanceTypeConfigsEbsConfigParameters struct {
	Iops *int64 `json:"iops,omitempty" tf:"iops"`

	Size int64 `json:"size" tf:"size"`

	Type string `json:"type" tf:"type"`

	VolumesPerInstance *int64 `json:"volumesPerInstance,omitempty" tf:"volumes_per_instance"`
}

@muvaf
Copy link
Member Author

muvaf commented Aug 23, 2021

We could possibly reuse it but the one we need to reuse may not be the one named first. For example, ideally we'd like to use EbsConfigParameters every place possible as long as its structure matches. However, it could be slightly different than others and MasterInstanceGroupEbsConfigParameters might be the one that's common, so it'd use MasterInstanceGroupEbsConfigParameters type for, let's say, CoreInstanceGroupEbsConfig field which is sort of misleading.

Apart from that, we'd need to come up with an equality check for the types which is dangerous because even if the fields are same, their kubebuilder validation or comments could be different. So, I'd kind of feel safer with abundance of types given that Terraform schema doesn't do that optimization either, i.e. letting author specify a schema and refer to it from several places.

Copy link
Member

@turkenh turkenh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@muvaf muvaf merged commit 03ca4b1 into crossplane:main Aug 23, 2021
@muvaf muvaf deleted the fix-sort branch August 23, 2021 17:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type collision in the same CRD package results in unstable output
2 participants