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

Only add singleton list API converters for the existing APIs #767

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

ulucinar
Copy link
Collaborator

@ulucinar ulucinar commented Jun 14, 2024

Description of your changes

Only apply the CRD API conversions for converting between singleton list & embedded object APIs for the already existing resources with the generated singleton list CRD APIs.

When we try to add a new Terraform resource with a singleton list in its schema, currently, we need to do a manual configuration such as the following:

p.AddResourceConfigurator("azurerm_pim_active_role_assignment", func(r *config.Resource) {
		r.PreviousVersions = nil
		r.Version = "v1beta1"
		r.Conversions = nil
		r.TerraformConversions = []config.TerraformConversion{
			config.NewTFSingletonConversion(),
		}
	})

, so that the API converters registered are removed and versioning starts with v1beta1 instead of v1beta2 (for such new resources, the v1beta1 version does not already exist).

This PR records the names of resources with previous CRD APIs containing (now converted) singleton lists so that the API converters are only registered for them.
This makes adding new resources with singleton lists in their Terraform schemas easier by removing the need to do a manual configuration like shown above.

The newly introduced list should mostly be immutable (it records a set of existing resources with a specific condition at a point in time). Bugs may result in exceptions where we may need to update this list. In other words, new resources should normally be never added to this list, doing so will result in an invalid configuration.

This PR also removes the relevant manual configuration done for the azurerm_pim_eligible_role_assignment & azurerm_pim_active_role_assignment Terraform resources.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Via an uptest run for the Subnet.v1beta2.network resource here. Also observed no configuration changes* for the PimActiveRoleAssignment.authorization & PimEligibleRoleAssignment.authorization resources.

*: The only configuration change is an identity API conversion being registered between all the API versions of these two resources (previously the manual conversion had deleted all API conversions including the identity conversion). However, these two resources each have single CRD API versions (v1beta1) so the API conversion configuration is effectively a noop, and the identity conversion is by default set for all resources.

…& embedded object APIs

for the already existing resources with generated singleton list CRD APIs.

- This makes adding new resources with singleton lists in their Terraform schemas
  easier by removing the need to do a manual configuration.

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
@ulucinar ulucinar force-pushed the selective-sl-config branch from c90cc3d to 3926a99 Compare June 14, 2024 16:26
@ulucinar
Copy link
Collaborator Author

/test-examples="examples/network/v1beta2/subnet.yaml"

Copy link
Collaborator

@mergenci mergenci left a comment

Choose a reason for hiding this comment

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

Thanks @ulucinar. LGTM. I'm leaving a reference to the counterpart PR here: crossplane-contrib/provider-upjet-aws#1367

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants