-
Notifications
You must be signed in to change notification settings - Fork 119
add type, name and id to properties accessible. #10932
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -177,7 +177,7 @@ func (c *CreateOrUpdateResource[P, T]) executeRecipeIfNeeded(ctx context.Context | |
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get connected resource IDs: %w", err) | ||
| } | ||
| connectedResourcesProperties := make(map[string]map[string]any) | ||
| connectedResourcesProperties := make(map[string]recipes.ConnectedResource) | ||
|
|
||
| // If there are connected resources, we need to fetch their properties and add them to the recipe context. | ||
| for connName, connectedResourceID := range connectionsAndSourceIDs { | ||
|
|
@@ -188,12 +188,17 @@ func (c *CreateOrUpdateResource[P, T]) executeRecipeIfNeeded(ctx context.Context | |
| return nil, fmt.Errorf("failed to get connected resource %s: %w", connectedResourceID, err) | ||
| } | ||
|
|
||
| connectedResourceProperties, err := resourceutil.GetPropertiesFromResource(connectedResource.Data) | ||
| connectedResourceMetadata, err := resourceutil.GetAllPropertiesFromResource(connectedResource.Data, connectedResourceID) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. The data is only the properties. Therefore there is no connectedResource.ID available. ID belongs one lveel outside in our modelling, but zach had a usecase where he needed the name, type and ID.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed GetAllPropertiesFromResource implementation to use resource's ID. |
||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get properties from connected resource %s: %w", connectedResourceID, err) | ||
| return nil, fmt.Errorf("failed to get metadata from connected resource %s: %w", connectedResourceID, err) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function is named
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we did since now we get properties + (name, type, id) |
||
| } | ||
|
|
||
| connectedResourcesProperties[connName] = connectedResourceProperties | ||
| connectedResourcesProperties[connName] = recipes.ConnectedResource{ | ||
| ID: connectedResourceMetadata.ID, | ||
| Name: connectedResourceMetadata.Name, | ||
| Type: connectedResourceMetadata.Type, | ||
| Properties: connectedResourceMetadata.Properties, | ||
| } | ||
| } | ||
|
|
||
| metadata := recipes.ResourceMetadata{ | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -368,7 +368,7 @@ func TestCreateOrUpdateResource_Run(t *testing.T) { | |||
| "p1": "v1", | ||||
| }, | ||||
| Properties: properties, | ||||
| ConnectedResourcesProperties: map[string]map[string]any{}, | ||||
| ConnectedResourcesProperties: map[string]recipes.ConnectedResource{}, | ||||
| } | ||||
|
|
||||
| prevState := []string{ | ||||
|
|
@@ -470,3 +470,34 @@ func TestCreateOrUpdateResource_Run(t *testing.T) { | |||
| }) | ||||
| } | ||||
| } | ||||
|
|
||||
| func TestGetAllPropertiesFromResource_Integration(t *testing.T) { | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does the |
||||
| // Simple integration test for resourceutil.GetAllPropertiesFromResource | ||||
| connectedResourceID := "/planes/radius/local/resourceGroups/radius-test-rg/providers/Applications.Datastores/sqlDatabases/test-db" | ||||
|
||||
|
|
||||
| // Create a test resource with properties | ||||
| testResource := struct { | ||||
| Properties map[string]any `json:"properties"` | ||||
| }{ | ||||
| Properties: map[string]any{ | ||||
| "host": "localhost", | ||||
| "port": 5432, | ||||
| "database": "testdb", | ||||
| }, | ||||
| } | ||||
|
|
||||
| // Test the GetAllPropertiesFromResource function | ||||
| metadata, err := resourceutil.GetAllPropertiesFromResource(testResource, connectedResourceID) | ||||
|
|
||||
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,9 +61,14 @@ func Test_Engine_Execute_Success(t *testing.T) { | |
| Parameters: map[string]any{ | ||
| "resourceName": "resource1", | ||
| }, | ||
| ConnectedResourcesProperties: map[string]map[string]any{ | ||
| ConnectedResourcesProperties: map[string]recipes.ConnectedResource{ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to do some assertions here? |
||
| "database": { | ||
| "name": "db", | ||
| ID: "/planes/radius/local/resourceGroups/radius-test-rg/providers/Applications.Datastores/sqlDatabases/database", | ||
| Name: "database", | ||
| Type: "Applications.Datastores/sqlDatabases", | ||
| Properties: map[string]any{ | ||
| "name": "db", | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -304,3 +304,82 @@ func TestNewContext_failures(t *testing.T) { | |
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestNewContext_WithConnectedResources(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to pay attention to our naming conventions and rename this test if necessary.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we also use table-driven if we have more cases to test? |
||
| testMetadata := &recipes.ResourceMetadata{ | ||
| ResourceID: "/planes/radius/local/resourceGroups/testGroup/providers/applications.datastores/mongodatabases/mongo0", | ||
| ApplicationID: "/planes/radius/local/resourceGroups/testGroup/providers/applications.core/applications/testApplication", | ||
| EnvironmentID: "/planes/radius/local/resourceGroups/testGroup/providers/applications.core/environments/testEnvironment", | ||
| Name: "recipe0", | ||
| Properties: map[string]any{ | ||
| "property1": "value1", | ||
| }, | ||
| ConnectedResourcesProperties: map[string]recipes.ConnectedResource{ | ||
| "database": { | ||
| ID: "/planes/radius/local/resourceGroups/testGroup/providers/applications.datastores/sqldatabases/testdb", | ||
| Name: "testdb", | ||
| Type: "Applications.Datastores/sqlDatabases", | ||
| Properties: map[string]any{ | ||
| "host": "localhost", | ||
| "port": 5432, | ||
| "database": "testdb", | ||
| }, | ||
| }, | ||
| "cache": { | ||
| ID: "/planes/radius/local/resourceGroups/testGroup/providers/applications.datastores/redis/testcache", | ||
| Name: "testcache", | ||
| Type: "Applications.Datastores/redis", | ||
| Properties: map[string]any{ | ||
| "host": "cache-host", | ||
| "port": 6379, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| testConfig := &recipes.Configuration{ | ||
| Providers: coredm.Providers{ | ||
| Azure: coredm.ProvidersAzure{ | ||
| Scope: "/planes/radius/local/resourceGroups/testGroup", | ||
| }, | ||
| AWS: coredm.ProvidersAWS{ | ||
| Scope: "/planes/aws/aws/accounts/1234567890/regions/us-west-2", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| result, err := New(testMetadata, testConfig) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, result) | ||
|
|
||
| // Verify basic resource properties | ||
| require.Equal(t, "mongo0", result.Resource.Name) | ||
| require.Equal(t, testMetadata.ResourceID, result.Resource.ID) | ||
| require.Equal(t, "applications.datastores/mongodatabases", result.Resource.Type) | ||
|
|
||
| // Verify connected resources metadata is available (connections are set by recipe engines) | ||
| require.NotNil(t, testMetadata.ConnectedResourcesProperties) | ||
| require.Len(t, testMetadata.ConnectedResourcesProperties, 2) | ||
|
|
||
| // Verify the metadata contains the expected connected resources | ||
| dbConn, exists := testMetadata.ConnectedResourcesProperties["database"] | ||
| require.True(t, exists) | ||
| require.Equal(t, "/planes/radius/local/resourceGroups/testGroup/providers/applications.datastores/sqldatabases/testdb", dbConn.ID) | ||
| require.Equal(t, "testdb", dbConn.Name) | ||
| require.Equal(t, "Applications.Datastores/sqlDatabases", dbConn.Type) | ||
| require.Equal(t, map[string]any{ | ||
| "host": "localhost", | ||
| "port": 5432, | ||
| "database": "testdb", | ||
| }, dbConn.Properties) | ||
|
|
||
| cacheConn, exists := testMetadata.ConnectedResourcesProperties["cache"] | ||
| require.True(t, exists) | ||
| require.Equal(t, "/planes/radius/local/resourceGroups/testGroup/providers/applications.datastores/redis/testcache", cacheConn.ID) | ||
| require.Equal(t, "testcache", cacheConn.Name) | ||
| require.Equal(t, "Applications.Datastores/redis", cacheConn.Type) | ||
| require.Equal(t, map[string]any{ | ||
| "host": "cache-host", | ||
| "port": 6379, | ||
| }, cacheConn.Properties) | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -55,10 +55,13 @@ type Resource struct { | |||||
| Properties map[string]any `json:"properties,omitempty"` | ||||||
|
|
||||||
| // Connections represent a map of connections to other resources. | ||||||
|
||||||
| // The key is the connection name, and the value is a map of connected resource properties. | ||||||
| // We enrich the recipe context with this, allowing the recipe to access properties of connected resources using the following format: | ||||||
| // context.resource.connections.[connection-name].[connected-resource-property] | ||||||
| Connections map[string]map[string]any `json:"connections,omitempty"` | ||||||
| // The key is the connection name, and the value contains the connected resource's metadata and properties. | ||||||
| // We enrich the recipe context with this, allowing the recipe to access connected resource info using: | ||||||
| // context.resource.connections.[connection-name].properties.[property-name] | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be as follows:
Suggested change
?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no.. we need the dot. |
||||||
| // context.resource.connections.[connection-name].id | ||||||
| // context.resource.connections.[connection-name].name | ||||||
| // context.resource.connections.[connection-name].type | ||||||
| Connections map[string]recipes.ConnectedResource `json:"connections,omitempty"` | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @zachcasper @sk593 , the comment captures the change to connections. please review. |
||||||
| } | ||||||
|
|
||||||
| // ResourceInfo represents name and id of the resource | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,9 +43,14 @@ func TestGenerateConfig(t *testing.T) { | |
| TemplatePath: "test/module/source", | ||
| }, | ||
| ResourceRecipe: &recipes.ResourceMetadata{ | ||
| ConnectedResourcesProperties: map[string]map[string]any{ | ||
| ConnectedResourcesProperties: map[string]recipes.ConnectedResource{ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to do some assertions? |
||
| "conn1": { | ||
| "dbName": "db", | ||
| ID: "/planes/radius/local/resourceGroups/radius-test-rg/providers/Applications.Datastores/redis/redis", | ||
| Name: "redis", | ||
| Type: "Applications.Datastores/redis", | ||
| Properties: map[string]any{ | ||
| "dbName": "db", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -24,6 +24,18 @@ import ( | |||||
| rpv1 "github.com/radius-project/radius/pkg/rp/v1" | ||||||
| ) | ||||||
|
|
||||||
| // ConnectedResource represents a connected resource's metadata and properties | ||||||
|
||||||
| // ConnectedResource represents a connected resource's metadata and properties | |
| // ConnectedResource represents a connected resource's metadata and properties. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,3 +92,34 @@ func GetConnectionNameandSourceIDs[P any](resource P) (map[string]string, error) | |
|
|
||
| return connectionNamesAndSourceIDs, nil | ||
| } | ||
|
|
||
| // ResourceMetadata represents resource metadata including ID, Name, Type and Properties | ||
| type ResourceMetadata struct { | ||
| ID string `json:"id"` | ||
| Name string `json:"name"` | ||
| Type string `json:"type"` | ||
| Properties map[string]any `json:"properties,omitempty"` | ||
| } | ||
|
Comment on lines
+96
to
+102
|
||
|
|
||
| // GetAllPropertiesFromResource extracts the resource metadata including ID, Name, Type and properties | ||
| // by parsing the resource ID and extracting properties. | ||
| func GetAllPropertiesFromResource[P any](resource P, resourceID string) (*ResourceMetadata, error) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the If not, is it the target resource or somethin like that? We need clearer argument names, I believe.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. It is because the properies bag does not include the resource ID. ID is one level higher, as part of trackedresources struct.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed the implementation so it is clearer. Please review. |
||
| // Parse resource ID to get name and type | ||
| parsedResourceID, err := resources.Parse(resourceID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse resource ID %s: %w", resourceID, err) | ||
| } | ||
|
|
||
| // Get properties using existing method | ||
| properties, err := GetPropertiesFromResource(resource) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &ResourceMetadata{ | ||
| ID: resourceID, | ||
| Name: parsedResourceID.Name(), | ||
| Type: parsedResourceID.Type(), | ||
| Properties: properties, | ||
| }, nil | ||
|
Comment on lines
+119
to
+124
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the difference between
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ID is a string. parsedResourceID is what we get we we do a resources.Parse(id) |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be renamed to just
connectedResources? Do we need to havePropertiespostfix?