Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/rad/cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -381,8 +381,8 @@ func initSubCommands() {

envDeleteCmd, _ := env_delete.NewCommand(framework)
previewDeleteCmd, _ := env_delete_preview.NewCommand(framework)
wirePreviewSubcommand(envDeleteCmd, previewDeleteCmd)
envCmd.AddCommand(envDeleteCmd)
wirePreviewSubcommandPreviewBase(previewDeleteCmd, envDeleteCmd.RunE, "Use the Radius.Core preview implementation for environment delete", "force")
Comment thread
lakshmimsft marked this conversation as resolved.
envCmd.AddCommand(previewDeleteCmd)

envListCmd, _ := env_list.NewCommand(framework)
previewListCmd, _ := env_list_preview.NewCommand(framework)
Expand Down
31 changes: 31 additions & 0 deletions cmd/rad/cmd/root_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,36 @@ func Test_ResourceList_ExposesPreviewFlag(t *testing.T) {
require.NotNil(t, listCmd.Flags().Lookup("preview"), "rad resource list must expose --preview")
}

// Test_EnvDelete_ExposesPreviewAndForceFlags asserts against the fully-assembled command tree that
// `rad env delete` exposes both flags. The preview runner reads --force during Validate, so if the
// wired command does not declare it, every `rad env delete --preview` invocation fails with
// "flag accessed but not defined: force".
func Test_EnvDelete_ExposesPreviewAndForceFlags(t *testing.T) {
deleteCmd, _, err := RootCmd.Find([]string{"env", "delete"})
require.NoError(t, err)
require.Equal(t, "delete", deleteCmd.Name())
require.NotNil(t, deleteCmd.Flags().Lookup("preview"), "rad env delete must expose --preview")
require.NotNil(t, deleteCmd.Flags().Lookup("force"), "rad env delete must expose --force")
Comment thread
lakshmimsft marked this conversation as resolved.
}

// Test_EnvDelete_ExposesFlagsLegacyRunnerReads guards the coupling created by registering the
// preview command as the base for `rad env delete`: the legacy runner now validates against the
// preview command's flag set. If any of these flags is dropped from the preview command, plain
// `rad env delete` (without --preview) breaks at runtime with "flag accessed but not defined".
// Test_EnvPreviewOnlyFlagsRejectedWithoutPreview cannot catch this, because it returns in the
// --force guard before it ever reaches the legacy runner.
func Test_EnvDelete_ExposesFlagsLegacyRunnerReads(t *testing.T) {
deleteCmd, _, err := RootCmd.Find([]string{"env", "delete"})
require.NoError(t, err)

// Read by the legacy runner's Validate, directly or via cli.RequireWorkspace,
// cli.RequireEnvironmentNameArgs and cli.RequireOutput.
for _, flag := range []string{"workspace", "group", "environment", "yes", "output"} {
require.NotNilf(t, deleteCmd.Flags().Lookup(flag),
"rad env delete must expose --%s: the legacy runner reads it during Validate", flag)
}
}

// Test_EnvPreviewOnlyFlagsRejectedWithoutPreview drives the real, fully-assembled command tree
// and asserts that each preview-only flag is rejected when preview mode is off. This guards
// against a preview-only flag being added to a command without also being registered in the
Expand All @@ -452,6 +482,7 @@ func Test_EnvPreviewOnlyFlagsRejectedWithoutPreview(t *testing.T) {
{command: "create", flag: "recipe-packs", value: "p1"},
{command: "update", flag: "recipe-packs", value: "p1"},
{command: "update", flag: "clear-kubernetes", value: "true"},
{command: "delete", flag: "force", value: "true"},
}

for _, tc := range testcases {
Expand Down
8 changes: 8 additions & 0 deletions pkg/cli/clients/clients.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,14 @@ type ApplicationsManagementClient interface {
// Names target Applications.Core; use a full resource ID for other environment types.
ListResourcesInEnvironment(ctx context.Context, environmentNameOrID string) ([]generated.GenericResource, error)

// ListResourcesInEnvironmentOrApplications lists the resources that belong to the given
// environment or to any of the given applications, without duplicates.
//
// This is equivalent to merging ListResourcesInEnvironment with ListResourcesInApplication for
// each application, but enumerates each resource type once instead of once per application.
// Names target Applications.Core; use full resource IDs for other environment and application types.
ListResourcesInEnvironmentOrApplications(ctx context.Context, environmentNameOrID string, applicationNameOrIDs []string) ([]generated.GenericResource, error)

// GetResource retrieves a resource by its type and name (or id).
GetResource(ctx context.Context, resourceType string, resourceNameOrID string) (generated.GenericResource, error)

Expand Down
62 changes: 62 additions & 0 deletions pkg/cli/clients/management.go
Original file line number Diff line number Diff line change
Expand Up @@ -1162,6 +1162,68 @@ func (amc *UCPApplicationsManagementClient) ListResourcesInEnvironment(ctx conte
return results, nil
}

// ListResourcesInEnvironmentOrApplications lists the resources that belong to the given environment
// or to any of the given applications, without duplicates.
//
// This produces the same set as merging ListResourcesInEnvironment with ListResourcesInApplication
// for each application, but enumerates each resource type once rather than once per application.
// The per-type listing returns every resource of that type in the scope and both membership checks
// run client side, so repeating the listing per application re-fetches identical data.
func (amc *UCPApplicationsManagementClient) ListResourcesInEnvironmentOrApplications(ctx context.Context, environmentNameOrID string, applicationNameOrIDs []string) ([]generated.GenericResource, error) {
environmentID, err := amc.fullyQualifyID(environmentNameOrID, "Applications.Core/environments")
if err != nil {
return nil, err
}

applicationIDs := make([]string, 0, len(applicationNameOrIDs))
for _, applicationNameOrID := range applicationNameOrIDs {
applicationID, err := amc.fullyQualifyID(applicationNameOrID, "Applications.Core/applications")
if err != nil {
return nil, err
}

applicationIDs = append(applicationIDs, applicationID)
}

resourceTypesList, err := amc.ListAllResourceTypesNames(ctx, "local")
if err != nil {
return nil, err
}

results := []generated.GenericResource{}
for _, resourceType := range resourceTypesList {
resources, err := amc.ListResourcesOfType(ctx, resourceType)
if err != nil {
return nil, err
}

for _, resource := range resources {
if isResourceInEnvironmentOrApplications(resource, environmentID, applicationIDs) {
results = append(results, resource)
}
}
}

return results, nil
}

// isResourceInEnvironmentOrApplications reports whether the resource belongs to the given
// environment or to any of the given applications. A resource is appended at most once by the
// caller, so the two directions cannot produce duplicates.
func isResourceInEnvironmentOrApplications(resource generated.GenericResource, environmentID string, applicationIDs []string) bool {
if isResourceInEnvironment(resource, environmentID) {
return true
}

for _, applicationID := range applicationIDs {
if isResourceInApplication(resource, applicationID) {
return true
}
}

return false
}

// CreateOrUpdateResourceType creates or updates a resource type in the configured plane.
func (amc *UCPApplicationsManagementClient) CreateOrUpdateResourceType(ctx context.Context, planeName string, resourceProviderName string, resourceTypeName string, resource *ucpv20231001.ResourceTypeResource) (ucpv20231001.ResourceTypeResource, error) {
client, err := amc.createResourceTypeClient()
Expand Down
240 changes: 240 additions & 0 deletions pkg/cli/clients/management_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,80 @@ func Test_Resource(t *testing.T) {
require.Equal(t, expectedResourceList, resources)
})

// ListResourcesInEnvironmentOrApplications replaces one ListResourcesInEnvironment call plus one
// ListResourcesInApplication call per application with a single pass over the resource types.
// These cases pin the two properties the cascade delete depends on: the result is the union of
// both membership directions, and a resource matching both directions is returned only once.
newListEnvironmentOrApplicationsClient := func(t *testing.T) *UCPApplicationsManagementClient {
mockResourceClient := NewMockgenericResourceClient(gomock.NewController(t))
mockResourceProviderClient := NewMockresourceProviderClient(gomock.NewController(t))

client := createResourceAndResourceProviderClient(mockResourceClient, mockResourceProviderClient)

mockResourceProviderClient.EXPECT().NewListProviderSummariesPager("local", gomock.Any()).Return(pager(resourceProviderSummaryPages))
mockResourceClient.EXPECT().
NewListByRootScopePager(gomock.Any()).
Return(pager(listPages)).AnyTimes()
mockResourceProviderClient.EXPECT().
GetProviderSummary(gomock.Any(), "local", gomock.Any(), gomock.Any()).
DoAndReturn(func(ctx context.Context, plane string, providerName string, opts *ucp.ResourceProvidersClientGetProviderSummaryOptions) (ucp.ResourceProvidersClientGetProviderSummaryResponse, error) {
summary := findProviderSummary(providerName)
if summary != nil {
return ucp.ResourceProvidersClientGetProviderSummaryResponse{
ResourceProviderSummary: *summary,
}, nil
}

// Fallback for providers not in test data
return ucp.ResourceProvidersClientGetProviderSummaryResponse{
ResourceProviderSummary: ucp.ResourceProviderSummary{
Name: &providerName,
ResourceTypes: map[string]*ucp.ResourceProviderSummaryResourceType{
"resourceType" + string(providerName[len(providerName)-1]): {
APIVersions: map[string]*ucp.ResourceTypeSummaryResultAPIVersion{
version: {},
},
},
},
},
}, nil
}).AnyTimes()

return client
}

t.Run("ListResourcesInEnvironmentOrApplications", func(t *testing.T) {
client := newListEnvironmentOrApplicationsClient(t)

// test1 belongs to both the environment and the application, test2 only to the environment.
// test1 must appear exactly once even though both membership checks match it.
expectedResourceList := []generated.GenericResource{*listPages[0].Value[0], *listPages[0].Value[1]}

resources, err := client.ListResourcesInEnvironmentOrApplications(t.Context(), "test-environment", []string{"test-application"})
require.NoError(t, err)
require.Equal(t, expectedResourceList, resources)
})

t.Run("ListResourcesInEnvironmentOrApplications with no applications", func(t *testing.T) {
client := newListEnvironmentOrApplicationsClient(t)

// An environment with no applications must still return its own resources.
expectedResourceList := []generated.GenericResource{*listPages[0].Value[0], *listPages[0].Value[1]}

resources, err := client.ListResourcesInEnvironmentOrApplications(t.Context(), "test-environment", []string{})
require.NoError(t, err)
require.Equal(t, expectedResourceList, resources)
})

t.Run("ListResourcesInEnvironmentOrApplications ignores other environments and applications", func(t *testing.T) {
client := newListEnvironmentOrApplicationsClient(t)

// test3 and test4 live in a different scope, so neither membership check matches them.
resources, err := client.ListResourcesInEnvironmentOrApplications(t.Context(), "other-environment", []string{"other-application"})
require.NoError(t, err)
require.Empty(t, resources)
})

t.Run("GetResource", func(t *testing.T) {
ctrl := gomock.NewController(t)
mock := NewMockgenericResourceClient(ctrl)
Expand Down Expand Up @@ -2689,6 +2763,172 @@ func setCapture(ctx context.Context, response *http.Response) {
}
}

// Test_isResourceInApplication covers the ownership matching that rad app delete and
// rad env delete rely on to find the resources owned by an application. The match must be
// case-insensitive, because resource IDs are not case-normalized on the wire.
func Test_isResourceInApplication(t *testing.T) {
applicationID := "/planes/radius/local/resourceGroups/test-group/providers/Radius.Core/applications/test-app"

testcases := []struct {
name string
properties map[string]any
expected bool
}{
{
name: "exact match",
properties: map[string]any{"application": applicationID},
expected: true,
},
{
name: "case-insensitive match",
properties: map[string]any{"application": strings.ToUpper(applicationID)},
expected: true,
},
{
name: "different application",
properties: map[string]any{"application": applicationID + "-other"},
expected: false,
},
{
name: "no application property",
properties: map[string]any{},
expected: false,
},
{
name: "empty application property",
properties: map[string]any{"application": ""},
expected: false,
},
{
name: "non-string application property",
properties: map[string]any{"application": 42},
expected: false,
},
}

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
resource := generated.GenericResource{Properties: tc.properties}
require.Equal(t, tc.expected, isResourceInApplication(resource, applicationID))
})
}
}

// Test_isResourceInEnvironment covers the environment matching that rad env delete relies on
// to find the resources deployed into an environment.
func Test_isResourceInEnvironment(t *testing.T) {
environmentID := "/planes/radius/local/resourceGroups/test-group/providers/Radius.Core/environments/test-env"

testcases := []struct {
name string
properties map[string]any
expected bool
}{
{
name: "exact match",
properties: map[string]any{"environment": environmentID},
expected: true,
},
{
name: "case-insensitive match",
properties: map[string]any{"environment": strings.ToUpper(environmentID)},
expected: true,
},
{
name: "different environment",
properties: map[string]any{"environment": environmentID + "-other"},
expected: false,
},
{
name: "no environment property",
properties: map[string]any{},
expected: false,
},
}

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
resource := generated.GenericResource{Properties: tc.properties}
require.Equal(t, tc.expected, isResourceInEnvironment(resource, environmentID))
})
}
}

// Test_isResourceInEnvironmentOrApplications covers the combined membership check used by the
// single-pass listing behind rad env delete's cascade. A resource is in scope when it belongs to
// the environment or to any of the applications being deleted.
func Test_isResourceInEnvironmentOrApplications(t *testing.T) {
environmentID := "/planes/radius/local/resourceGroups/test-group/providers/Radius.Core/environments/test-env"
applicationID := "/planes/radius/local/resourceGroups/test-group/providers/Radius.Core/applications/test-app"
otherApplicationID := "/planes/radius/local/resourceGroups/test-group/providers/Radius.Core/applications/other-app"

testcases := []struct {
name string
properties map[string]any
applicationIDs []string
expected bool
}{
{
name: "environment match only",
properties: map[string]any{"environment": environmentID},
applicationIDs: []string{applicationID},
expected: true,
},
{
name: "application match only",
properties: map[string]any{"application": applicationID},
applicationIDs: []string{applicationID},
expected: true,
},
{
name: "matches a later application in the list",
properties: map[string]any{"application": applicationID},
applicationIDs: []string{otherApplicationID, applicationID},
expected: true,
},
{
name: "both directions match",
properties: map[string]any{"application": applicationID, "environment": environmentID},
applicationIDs: []string{applicationID},
expected: true,
},
{
name: "case-insensitive application match",
properties: map[string]any{"application": strings.ToUpper(applicationID)},
applicationIDs: []string{applicationID},
expected: true,
},
{
name: "environment match with no applications",
properties: map[string]any{"environment": environmentID},
applicationIDs: []string{},
expected: true,
},
{
name: "application match ignored when the application is not being deleted",
properties: map[string]any{"application": otherApplicationID},
applicationIDs: []string{applicationID},
expected: false,
},
{
name: "neither direction matches",
properties: map[string]any{"environment": environmentID + "-other"},
applicationIDs: []string{applicationID},
expected: false,
},
}

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
resource := generated.GenericResource{Properties: tc.properties}
require.Equal(t, tc.expected, isResourceInEnvironmentOrApplications(resource, environmentID, tc.applicationIDs))
})
}
}

func testCapture(ctx context.Context, capture **http.Response) context.Context {
return context.WithValue(ctx, holder{}, &holder{capture})
}
Loading
Loading