Adding Providers details for env show command with --preview flag #10925
Conversation
Signed-off-by: Vishwanath Hiremath <[email protected]>
vishwahiremat
requested a deployment
to
functional-tests
GitHub Actions
Waiting
vishwahiremat
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #10925 +/- ##
==========================================
- Coverage 50.43% 50.42% -0.02%
==========================================
Files 672 672
Lines 42057 42104 +47
==========================================
+ Hits 21213 21231 +18
- Misses 18788 18815 +27
- Partials 2056 2058 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
vishwahiremat
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
vishwahiremat
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
Radius functional test overviewClick here to see the test run details
Test Status⌛ Building Radius and pushing container images for functional tests... |
There was a problem hiding this comment.
Pull request overview
This pull request updates the AWS provider configuration schema to use separate accountId and region fields instead of a single scope field, making it consistent with the Azure provider design. It also adds provider details display to the rad env show --preview command output.
Key Changes
- Refactored AWS provider from single
scopeproperty to separateaccountIdandregionproperties for better consistency with Azure provider - Added provider information display to
rad env show --previewcommand - Updated TypeSpec definitions, OpenAPI specifications, datamodel structures, and all related conversion logic
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| typespec/Radius.Core/environments.tsp | Updated ProvidersAws model to use separate accountId and region fields instead of scope |
| swagger/specification/radius/resource-manager/Radius.Core/preview/2025-08-01-preview/openapi.json | Updated OpenAPI spec for ProvidersAws with new field structure and required fields |
| pkg/corerp/datamodel/environment_v20250801preview.go | Updated datamodel to reflect new AccountID and Region fields with JSON tags |
| pkg/corerp/api/v20250801preview/zz_generated_models.go | Updated generated API models for ProvidersAws structure |
| pkg/corerp/api/v20250801preview/zz_generated_models_serde.go | Updated JSON marshaling/unmarshaling for new AWS provider fields |
| pkg/corerp/api/v20250801preview/environment_conversion.go | Updated conversion logic between API and datamodel for AWS provider |
| pkg/recipes/configloader/environment.go | Updated to construct AWS scope from separate accountId and region fields |
| pkg/recipes/configloader/environment_test.go | Updated test data to use new AWS provider structure |
| pkg/cli/objectformats/objectformats.go | Added new table format function for displaying provider information |
| pkg/cli/cmd/env/show/preview/show.go | Added provider details display logic with formatting for Azure, AWS, and Kubernetes providers |
| pkg/cli/cmd/env/show/preview/show_test.go | Added test coverage for provider display functionality |
| pkg/cli/cmd/env/update/preview/update.go | Updated validation and update logic to handle separate AWS accountId and region fields; removed unused fmt import |
| pkg/cli/cmd/env/update/preview/update_test.go | Updated test data to use new AWS provider field structure |
| pkg/cli/test_client_factory/radius_core.go | Added provider configuration to test environment resource |
| hack/bicep-types-radius/generated/radius/radius.core/2025-08-01-preview/types.json | Updated Bicep types definition for AWS provider fields |
| // Provider is the type of the provider (e.g., "azure", "aws", "kubernetes"). | ||
| Provider string | ||
| // Properties contains the provider details in a comma-separated key-value format. | ||
| // e.g., "subscriptionId: 'sub-id', resourceGroupName: 'rg-name'" for azure provider." |
There was a problem hiding this comment.
There is an extra period at the end of the example comment that should be removed for consistency with other comment lines.
| // e.g., "subscriptionId: 'sub-id', resourceGroupName: 'rg-name'" for azure provider." | |
| // e.g., "subscriptionId: 'sub-id', resourceGroupName: 'rg-name'" for azure provider |
| envProviders := []EnvProviders{} | ||
| if resp.EnvironmentResource.Properties.Providers != nil { | ||
| if resp.EnvironmentResource.Properties.Providers.Azure != nil { | ||
| azureProvider := EnvProviders{ | ||
| Provider: "azure", | ||
| } | ||
| azureProvider.Properties = "subscriptionId: '" + *resp.EnvironmentResource.Properties.Providers.Azure.SubscriptionID + "', resourceGroupName: '" + *resp.EnvironmentResource.Properties.Providers.Azure.ResourceGroupName + "'" | ||
| envProviders = append(envProviders, azureProvider) | ||
| } | ||
|
|
||
| if resp.EnvironmentResource.Properties.Providers.Aws != nil { | ||
| awsProvider := EnvProviders{ | ||
| Provider: "aws", | ||
| } | ||
| awsProvider.Properties = "accountId: '" + *resp.EnvironmentResource.Properties.Providers.Aws.AccountID + "', region: '" + *resp.EnvironmentResource.Properties.Providers.Aws.Region + "'" | ||
| envProviders = append(envProviders, awsProvider) | ||
| } | ||
| if resp.EnvironmentResource.Properties.Providers.Kubernetes != nil { | ||
| k8sProvider := EnvProviders{ | ||
| Provider: "kubernetes", | ||
| } | ||
| k8sProvider.Properties = "namespace: '" + *resp.EnvironmentResource.Properties.Providers.Kubernetes.Namespace + "'" | ||
| envProviders = append(envProviders, k8sProvider) | ||
| } | ||
| } |
There was a problem hiding this comment.
The provider formatting logic is repetitive and could be refactored into a helper function to reduce code duplication. Consider extracting a function that takes provider type, field names, and values to construct the Properties string. This would make the code more maintainable and reduce the risk of inconsistent formatting across providers.
| azureProvider := EnvProviders{ | ||
| Provider: "azure", | ||
| } | ||
| azureProvider.Properties = "subscriptionId: '" + *resp.EnvironmentResource.Properties.Providers.Azure.SubscriptionID + "', resourceGroupName: '" + *resp.EnvironmentResource.Properties.Providers.Azure.ResourceGroupName + "'" |
There was a problem hiding this comment.
Potential nil pointer dereference when accessing provider properties. If any of the pointer fields (SubscriptionID, ResourceGroupName, AccountID, Region, or Namespace) are nil, dereferencing them will cause a panic. Add nil checks before dereferencing these pointers or use helper functions to safely access pointer values.
| awsProvider := EnvProviders{ | ||
| Provider: "aws", | ||
| } | ||
| awsProvider.Properties = "accountId: '" + *resp.EnvironmentResource.Properties.Providers.Aws.AccountID + "', region: '" + *resp.EnvironmentResource.Properties.Providers.Aws.Region + "'" |
There was a problem hiding this comment.
Potential nil pointer dereference when accessing AWS provider AccountID and Region fields. If either field is nil, dereferencing will cause a panic. Add nil checks before dereferencing these pointers or use helper functions to safely access pointer values.
| awsProvider.Properties = "accountId: '" + *resp.EnvironmentResource.Properties.Providers.Aws.AccountID + "', region: '" + *resp.EnvironmentResource.Properties.Providers.Aws.Region + "'" | |
| accountID := "" | |
| if resp.EnvironmentResource.Properties.Providers.Aws.AccountID != nil { | |
| accountID = *resp.EnvironmentResource.Properties.Providers.Aws.AccountID | |
| } | |
| region := "" | |
| if resp.EnvironmentResource.Properties.Providers.Aws.Region != nil { | |
| region = *resp.EnvironmentResource.Properties.Providers.Aws.Region | |
| } | |
| awsProvider.Properties = "accountId: '" + accountID + "', region: '" + region + "'" |
| k8sProvider := EnvProviders{ | ||
| Provider: "kubernetes", | ||
| } | ||
| k8sProvider.Properties = "namespace: '" + *resp.EnvironmentResource.Properties.Providers.Kubernetes.Namespace + "'" |
There was a problem hiding this comment.
Potential nil pointer dereference when accessing Kubernetes Namespace field. If the Namespace field is nil, dereferencing will cause a panic. Add nil checks before dereferencing this pointer or use helper functions to safely access pointer values.
| k8sProvider.Properties = "namespace: '" + *resp.EnvironmentResource.Properties.Providers.Kubernetes.Namespace + "'" | |
| namespace := "" | |
| if resp.EnvironmentResource.Properties.Providers.Kubernetes.Namespace != nil { | |
| namespace = *resp.EnvironmentResource.Properties.Providers.Kubernetes.Namespace | |
| } | |
| k8sProvider.Properties = "namespace: '" + namespace + "'" |
| azureProvider := EnvProviders{ | ||
| Provider: "azure", | ||
| } | ||
| azureProvider.Properties = "subscriptionId: '" + *resp.EnvironmentResource.Properties.Providers.Azure.SubscriptionID + "', resourceGroupName: '" + *resp.EnvironmentResource.Properties.Providers.Azure.ResourceGroupName + "'" |
There was a problem hiding this comment.
Using direct string concatenation with the + operator. For better performance and idiomatic Go code, use strings.Builder or fmt.Sprintf when building strings from multiple parts. This is particularly important since this code constructs display strings for potentially multiple providers.
| awsProvider := EnvProviders{ | ||
| Provider: "aws", | ||
| } | ||
| awsProvider.Properties = "accountId: '" + *resp.EnvironmentResource.Properties.Providers.Aws.AccountID + "', region: '" + *resp.EnvironmentResource.Properties.Providers.Aws.Region + "'" |
There was a problem hiding this comment.
Using direct string concatenation with the + operator. For better performance and idiomatic Go code, use strings.Builder or fmt.Sprintf when building strings from multiple parts. This is particularly important since this code constructs display strings for potentially multiple providers.
| if envDatamodel.Properties.Providers.AWS != nil { | ||
| config.Providers.AWS = datamodel.ProvidersAWS{ | ||
| Scope: envDatamodel.Properties.Providers.AWS.Scope, | ||
| Scope: "/planes/aws/aws/accounts/" + envDatamodel.Properties.Providers.AWS.AccountID + "/regions/" + envDatamodel.Properties.Providers.AWS.Region, |
There was a problem hiding this comment.
Using direct string concatenation with the + operator to construct the AWS scope path. For better performance and idiomatic Go code, use fmt.Sprintf or strings.Builder when building strings from multiple parts. Consider using fmt.Sprintf with a format string like "/planes/aws/aws/accounts/%s/regions/%s" for clarity.
| k8sProvider := EnvProviders{ | ||
| Provider: "kubernetes", | ||
| } | ||
| k8sProvider.Properties = "namespace: '" + *resp.EnvironmentResource.Properties.Providers.Kubernetes.Namespace + "'" |
There was a problem hiding this comment.
Using direct string concatenation with the + operator. For better performance and idiomatic Go code, use strings.Builder or fmt.Sprintf when building strings from multiple parts. This is particularly important since this code constructs display strings for potentially multiple providers.
| Providers: &corerpv20250801.Providers{ | ||
| Azure: &corerpv20250801.ProvidersAzure{ | ||
| SubscriptionID: to.Ptr("test-subscription-id"), | ||
| ResourceGroupName: to.Ptr("test-resource-group"), | ||
| }, | ||
| Aws: &corerpv20250801.ProvidersAws{ | ||
| AccountID: to.Ptr("test-account-id"), | ||
| Region: to.Ptr("test-region"), | ||
| }, | ||
| Kubernetes: &corerpv20250801.ProvidersKubernetes{ | ||
| Namespace: to.Ptr("test-namespace"), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Test coverage is missing for scenarios where provider fields could be nil. The new code in show.go dereferences pointer fields without nil checks (lines 154, 162, 169), which could cause panics. Add test cases covering environments with partially configured providers or nil pointer fields to ensure the code handles these edge cases gracefully.
1 participant
Description
Added Providers details for env show command with --preview flag.
rad env show --previewcommand to add provider details.Updated the Aws provider properties
scopeproperty and made it consistent with Azure provider by adding propertiesaccoutIdandregionseparatelyHow to Test:
sudo make installto get the latest cli changesType of change
#9832
Contributor checklist
Please verify that the PR meets the following requirements, where applicable: