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

Add support for 'ref' annotation #210

Merged

Conversation

nimrodshn
Copy link
Collaborator

@nimrodshn nimrodshn commented Dec 2, 2024

What this does?

Adds support for a reference directive - @ref(name="/path/to/service") in order to share resources and definitions between services.

Currently this is only supported for types in order to share their definition between services, however, as we continue to implement the /aro_hcp service we will likely enhance this implementation.

Here is an example use case where we share the Cluster type:

@ref(path = "/clusters_mgmt/v1/cluster")
class Cluster {
}

With sdk that looks like the following:

It("Check for duplicate cluster name with same Azure Resource Name", func() {
	postBody, err := arohcphelper.GetAroHCPClusterBuilder(azureBuilder, westUs3, properties).Build()
	helper.CheckError(err)
	postResponse, err := connection.AroHCP().V1Alpha1().Clusters().Add().Body(postBody).SendContext(ctx)
	helper.CheckErrorResponse(postResponse, err, http.StatusBadRequest)
	Expect(postResponse.Error().Reason()).To(ContainSubstring("Duplicate ARO-HCP cluster"))
})

Important detail is that this preserves the "links" or references to other resources unless defined explicitly, for example:

func GetAroHCPClusterBuilder(azureBuilder *arohcp.AzureBuilder,
	region string, properties map[string]string) *arohcp.ClusterBuilder {
	return arohcp.NewCluster().
		Name(helper.GenerateClusterName()).
		Product(cmv1.NewProduct().ID(models.AROProductID)).
		CCS(arohcp.NewCCS().Enabled(true)).
		Region(cmv1.NewCloudRegion().ID(region)).
		Hypershift(arohcp.NewHypershift().Enabled(true)).
		MultiAZ(true).
		Azure(azureBuilder).
		Properties(properties)
}

From the docs [committed here]:

Class references using @ref annotation

One can define a class reference inside a service such that it will inherit its content from
another service using the special @ref annotation, for example:

// In /aro_hcp/v1_alpha1/cluster_type.model
@ref(path = "/clusters_mgmt/v1/cluster")
class Cluster {
}

The above decelaration will inherit its content from the Cluster class under the /clusters_mgmt/v1 service.
This means that any changes made in Cluster class will be reflected under this derived type as well.

Links to other resources are preserved as they are.

If one wishes to override a type she can create a class inside /aro_hcp/v1_alpha1/ in order to override any nested types defined.
For example the following type declaration will override the NodePools link inside Cluster under /aro_hcp/v1:

// In /aro_hcp/v1_alpha1/node_pool_type.model
@ref(path = "/clusters_mgmt/v1/node_pool")
class NodePool {
}

This means that now under Cluster the NodePools field type is linked to the one defined in /aro_hcp/v1_alpha1 (i.e. v1alpha.NodePool) which itself references and derived from the NodePool type under /clusters_mgmt/v1.

@nimrodshn nimrodshn force-pushed the add_reference_to_service_locator branch 2 times, most recently from 9635173 to b8b62a0 Compare December 3, 2024 12:40
@zgalor
Copy link
Collaborator

zgalor commented Dec 3, 2024

LGTM, but would like to have more eyes on this
@tzvatot @miguelsorianod

@tzvatot
Copy link
Contributor

tzvatot commented Dec 3, 2024

It seems that the "ref" value is mixing the URL endpoint (@ref(path = "/clusters_mgmt/v1/cluster")) with the struct definition. Same struct can be shared on different endpoint.

How about to make the path binded to the actual class definition path, rather than the endpoint itself, which can change.

@nimrodshn
Copy link
Collaborator Author

nimrodshn commented Dec 3, 2024

@tzvatot You're correct. This implementation is somewhat naive, we are now enhancing this mechanism to bind the struct like you suggested.

@nimrodshn nimrodshn force-pushed the add_reference_to_service_locator branch from b8b62a0 to c64d3a7 Compare December 3, 2024 15:14
@nimrodshn nimrodshn force-pushed the add_reference_to_service_locator branch from c64d3a7 to ab9daad Compare December 4, 2024 05:58
@nimrodshn
Copy link
Collaborator Author

@tzvatot PTAL - I've changed the implementation as per you're suggestion. You can see in the here a fully working example.

@nimrodshn nimrodshn force-pushed the add_reference_to_service_locator branch from b5183a1 to a676eb2 Compare December 4, 2024 07:31
@nimrodshn nimrodshn force-pushed the add_reference_to_service_locator branch 10 times, most recently from 8f7c7ee to 1af7b0d Compare December 8, 2024 20:50
pkg/language/reader.go Outdated Show resolved Hide resolved
pkg/language/reader.go Outdated Show resolved Hide resolved
@nimrodshn nimrodshn force-pushed the add_reference_to_service_locator branch 2 times, most recently from 3f1f0d8 to 342522c Compare December 9, 2024 14:05
@nimrodshn nimrodshn force-pushed the add_reference_to_service_locator branch from 342522c to 3fc89e6 Compare December 9, 2024 14:07
@tzvatot
Copy link
Contributor

tzvatot commented Dec 9, 2024

Please add UT

@nimrodshn nimrodshn force-pushed the add_reference_to_service_locator branch from 97ac562 to 34762d8 Compare December 10, 2024 09:52
@nimrodshn nimrodshn force-pushed the add_reference_to_service_locator branch 4 times, most recently from ddaf15c to 017e2a7 Compare December 10, 2024 20:06
@nimrodshn nimrodshn force-pushed the add_reference_to_service_locator branch from 017e2a7 to e4374af Compare December 11, 2024 10:27
@zgalor
Copy link
Collaborator

zgalor commented Dec 11, 2024

lgtm

Copy link
Contributor

@ahitacat ahitacat left a comment

Choose a reason for hiding this comment

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

lgtm

@nimrodshn
Copy link
Collaborator Author

@jhernand Mind merging please?

@jhernand jhernand merged commit e4559f2 into openshift-online:main Dec 12, 2024
1 check passed
@jhernand
Copy link
Collaborator

@jhernand Mind merging please?

Done, and you should have permissions now.

@nimrodshn
Copy link
Collaborator Author

Many thanks @jhernand !

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.

6 participants