support deployment of an application to a scope "scope1" using environment in a different scope "scope2" #7895

nithyatsu · 2024-09-07T02:02:01Z

Description

Today, rad deploy can use only the environments that are in same scope as where the application is being deployed to (either default scope or one specified by -g).
It should be able to use environments in different groups to deploy application.

Type of change

This pull request fixes a bug in Radius and has an approved issue (issue link required).
Fixes: rad deploy should be able to use environments in different groups to deploy application #7520

codecov · 2024-09-07T02:11:59Z

Codecov Report

Attention: Patch coverage is 48.00000% with 13 lines in your changes missing coverage. Please review.

Project coverage is 60.97%. Comparing base (3d2825f) to head (e7c96e7).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/cli/clivalidation.go	0.00%	13 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7895      +/-   ##
==========================================
- Coverage   60.98%   60.97%   -0.02%     
==========================================
  Files         527      527              
  Lines       27844    27860      +16     
==========================================
+ Hits        16982    16988       +6     
- Misses       9367     9378      +11     
+ Partials     1495     1494       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ytimocin · 2024-09-09T17:20:44Z

pkg/cli/clivalidation.go

+	} else if environmentNameOrID == "" {
+		return "", fmt.Errorf("no environment name or ID provided, pass in an environment name or ID")
+	}


Does this mean that the workspace doesn't also have an environment? Should we add that to the error message?

The condition where workspace is editable and not set with env is checked at https://github.com/nithyatsu/radius/blob/1577a407a7079c53d56090882de2b74ceb2e99b8/pkg/cli/clivalidation.go#L120

This is because some of the cli code has the ability to make a fall back workspace to get to the server side (this just injects kubectx = default kubernetes context) and continue to work if config.yaml is not found. If this is the workspace we are dealing with, it is not editable. Then we cannot set env. L120 is the more common case.

ytimocin · 2024-09-09T17:21:38Z

pkg/cli/clivalidation.go

 // DidSpecifyEnvironmentName checks if an environment name is provided as a flag
 func DidSpecifyEnvironmentName(cmd *cobra.Command, args []string) bool {
 	environmentName, err := cmd.Flags().GetString("environment")
-	return err == nil && environmentName != ""
+	return err == nil && environmentName != "" && !strings.HasPrefix(environmentName, resources.SegmentSeparator)


Does this apply to both just the environment name and also the resource id for the environment?

No, it checks only for environment name. Since now we might be dealing with an id, it is updated to make sure we dont have the SegmentSeperator character that suggests this is an id.

Do we need DidSpecifyEnvironmentID?

No, there wasn't necessity for it.

ytimocin · 2024-09-09T17:24:23Z

pkg/cli/cmd/deploy/deploy.go

+# deploy using an environment ID and a resource group. The application will be deployed in mygroup scope, using the specified environment.
+# use this option if the environment is in a different group.
+rad deploy myapp.bicep --environment /planes/radius/local/resourcegroups/prod/providers/Applications.Core/environments/prod --group mygroup


Will we be able to pass in just a name? It says here like that: https://github.com/radius-project/radius/pull/7895/files#diff-bf4e917446c73565ee6967f28bdd8e619c0d4fa40b215f195b840c021dd36ce9R104.

Yes, we can still pass a name. In that case, the current group is searched for the environment (like before). But if we want to use an environment in a different group, then we would have to specify the ID so that we know the group.

Why do we need the resource group?

Can we also just do rad deploy myapp.bicep --environment /planes/radius/local/resourcegroups/prod/providers/Applications.Core/environments/prod?

Also, in the example the resource group in the environment ID is different than the resource group that is passed via a flag.

We should be more specific in these examples.

rad deploy myapp.bicep --environment /planes/radius/local/resourcegroups/prod/providers/Applications.Core/environments/prod --group mygroup

This command deploys the application to group mygroup using environment /planes/radius/local/resourcegroups/prod/providers/Applications.Core/environments/prod in the prod group.
--group flag existed already and specifies the group to which the application should be deployed to. There wasnt any ability to use an environment from another group though. But if we think about it, environments contain information used for deploying apps, and user should be able to use environments from any group.

Also, its OK to specify the --group value to be same group as in env ID. But in this case, name would be enough.

ytimocin · 2024-09-09T17:25:12Z

pkg/cli/cmd/deploy/deploy.go

 	if err != nil {
 		// If the error is not a 404, return it
 		if !clients.Is404Error(err) {
 			return err
 		}

-		// If the environment doesn't exist, but the user specified it as
+		// If the environment doesn't exist, but the user specified it's name as


Suggested change

// If the environment doesn't exist, but the user specified it's name as

// If the environment doesn't exist, but the user specified its name or resource id as

ytimocin · 2024-09-09T17:26:04Z

pkg/cli/cmd/deploy/deploy_test.go

@@ -53,7 +54,7 @@ func Test_Validate(t *testing.T) {
 			},
 			ConfigureMocks: func(mocks radcli.ValidateMocks) {
 				mocks.ApplicationManagementClient.EXPECT().
-					GetEnvironment(gomock.Any(), radcli.TestEnvironmentName).
+					GetEnvironment(gomock.Any(), "/planes/radius/local/resourceGroups/test-resource-group/providers/Applications.Core/environments/test-environment").


Should we have another test just with the name instead of the resource id?

I did not add it since most of the tests use a name and not ID for testing.. I will add one excplicitly for the name and rename this too to indicate the improtance of retaining the ID

sorry, I just noticed I covered it. They are "rad deploy - valid with environment" and "rad deploy - valid with env ID".

radius-functional-tests · 2024-09-09T19:46:51Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	nithyatsu/radius
Commit ref	`1577a40`
Unique ID	func8bc958b73d
Image tag	pr-func8bc958b73d

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr: 1.12.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.3.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func8bc958b73d
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-func8bc958b73d
controller test image location: ghcr.io/radius-project/dev/controller:pr-func8bc958b73d
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func8bc958b73d
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting datastoresrp-cloud functional tests...
⌛ Starting corerp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
✅ datastoresrp-cloud functional tests succeeded
✅ ucp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

radius-functional-tests · 2024-09-09T20:35:26Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	nithyatsu/radius
Commit ref	`aee1cca`
Unique ID	funcb5ac373a4c
Image tag	pr-funcb5ac373a4c

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr: 1.12.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.3.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-funcb5ac373a4c
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-funcb5ac373a4c
controller test image location: ghcr.io/radius-project/dev/controller:pr-funcb5ac373a4c
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-funcb5ac373a4c
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting corerp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
✅ datastoresrp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded
✅ ucp-cloud functional tests succeeded

Signed-off-by: nithyatsu <[email protected]>

radius-functional-tests · 2024-09-10T17:34:34Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	nithyatsu/radius
Commit ref	`e7c96e7`
Unique ID	func7dc83fa36e
Image tag	pr-func7dc83fa36e

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr: 1.12.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.3.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func7dc83fa36e
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-func7dc83fa36e
controller test image location: ghcr.io/radius-project/dev/controller:pr-func7dc83fa36e
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func7dc83fa36e
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting corerp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded
✅ datastoresrp-cloud functional tests succeeded

ytimocin · 2024-09-10T20:46:41Z

pkg/cli/clivalidation.go

 // DidSpecifyEnvironmentName checks if an environment name is provided as a flag
 func DidSpecifyEnvironmentName(cmd *cobra.Command, args []string) bool {
 	environmentName, err := cmd.Flags().GetString("environment")
-	return err == nil && environmentName != ""
+	return err == nil && environmentName != "" && !strings.HasPrefix(environmentName, resources.SegmentSeparator)


Do we need DidSpecifyEnvironmentID?

ytimocin · 2024-09-10T20:47:48Z

pkg/cli/cmd/deploy/deploy.go

+# deploy using an environment ID and a resource group. The application will be deployed in mygroup scope, using the specified environment.
+# use this option if the environment is in a different group.
+rad deploy myapp.bicep --environment /planes/radius/local/resourcegroups/prod/providers/Applications.Core/environments/prod --group mygroup


Why do we need the resource group?

ytimocin · 2024-09-10T20:48:09Z

pkg/cli/cmd/deploy/deploy.go

+# deploy using an environment ID and a resource group. The application will be deployed in mygroup scope, using the specified environment.
+# use this option if the environment is in a different group.
+rad deploy myapp.bicep --environment /planes/radius/local/resourcegroups/prod/providers/Applications.Core/environments/prod --group mygroup


Can we also just do rad deploy myapp.bicep --environment /planes/radius/local/resourcegroups/prod/providers/Applications.Core/environments/prod?

ytimocin · 2024-09-10T20:48:52Z

pkg/cli/cmd/deploy/deploy.go

+# deploy using an environment ID and a resource group. The application will be deployed in mygroup scope, using the specified environment.
+# use this option if the environment is in a different group.
+rad deploy myapp.bicep --environment /planes/radius/local/resourcegroups/prod/providers/Applications.Core/environments/prod --group mygroup


Also, in the example the resource group in the environment ID is different than the resource group that is passed via a flag.

We should be more specific in these examples.

ytimocin · 2024-09-10T20:49:36Z

pkg/cli/cmd/deploy/deploy.go

 		// a command-line option, return an error
 		if cli.DidSpecifyEnvironmentName(cmd, args) {
-			return clierrors.Message("The environment %q does not exist in scope %q. Run `rad env create` first.", r.EnvironmentName, r.Workspace.Scope)
+			return clierrors.Message("The environment %q does not exist in scope %q. Run `rad env create` first. You could also provide the environment ID if the environment exists in a different group.", r.EnvironmentNameOrID, r.Workspace.Scope)


Can we only provide an environmentID when we are deploying to another group?

Yes. If not, it will enter the codeblock if cli.DidSpecifyEnvironmentName and print the error message indicating the environment isnt found in group and we should specify the id if it exists in a different group.

nithyatsu changed the title ~~Cli bug~~ support deployment of an application to a scope "scope1" using environment in a different scope "scope2" Sep 7, 2024

nithyatsu marked this pull request as ready for review September 9, 2024 16:25

nithyatsu requested review from a team as code owners September 9, 2024 16:25

ytimocin reviewed Sep 9, 2024

View reviewed changes

nithyatsu temporarily deployed to functional-tests September 9, 2024 19:45 — with GitHub Actions Inactive

nithyatsu requested a review from ytimocin September 9, 2024 20:00

nithyatsu force-pushed the cli_bug branch from d84629b to aee1cca Compare September 9, 2024 20:33

nithyatsu temporarily deployed to functional-tests September 9, 2024 20:34 — with GitHub Actions Inactive

nithyatsu added 3 commits September 10, 2024 10:31

support using environment in different scope to deploy application

d594262

Signed-off-by: nithyatsu <[email protected]>

add test for environment ID support

1e64563

Signed-off-by: nithyatsu <[email protected]>

add comment

e7c96e7

Signed-off-by: nithyatsu <[email protected]>

nithyatsu force-pushed the cli_bug branch from aee1cca to e7c96e7 Compare September 10, 2024 17:31

nithyatsu temporarily deployed to functional-tests September 10, 2024 17:33 — with GitHub Actions Inactive

ytimocin reviewed Sep 10, 2024

View reviewed changes

ytimocin approved these changes Sep 10, 2024

View reviewed changes

nithyatsu merged commit 897f2aa into radius-project:main Sep 10, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support deployment of an application to a scope "scope1" using environment in a different scope "scope2" #7895

support deployment of an application to a scope "scope1" using environment in a different scope "scope2" #7895

nithyatsu commented Sep 7, 2024

codecov bot commented Sep 7, 2024 •

edited

Loading

ytimocin Sep 9, 2024

nithyatsu Sep 9, 2024

ytimocin Sep 9, 2024

nithyatsu Sep 9, 2024

ytimocin Sep 10, 2024

nithyatsu Sep 10, 2024

ytimocin Sep 9, 2024

nithyatsu Sep 9, 2024

ytimocin Sep 10, 2024

ytimocin Sep 10, 2024

ytimocin Sep 10, 2024

nithyatsu Sep 10, 2024 •

edited

Loading

nithyatsu Sep 10, 2024

ytimocin Sep 9, 2024

ytimocin Sep 9, 2024

nithyatsu Sep 9, 2024 •

edited

Loading

nithyatsu Sep 9, 2024

radius-functional-tests bot commented Sep 9, 2024 •

edited

Loading

radius-functional-tests bot commented Sep 9, 2024 •

edited

Loading

radius-functional-tests bot commented Sep 10, 2024 •

edited

Loading

ytimocin Sep 10, 2024

ytimocin Sep 10, 2024

ytimocin Sep 10, 2024

ytimocin Sep 10, 2024

ytimocin Sep 10, 2024

nithyatsu Sep 10, 2024 •

edited

Loading

	// If the environment doesn't exist, but the user specified it's name as
	// If the environment doesn't exist, but the user specified its name or resource id as

support deployment of an application to a scope "scope1" using environment in a different scope "scope2" #7895

support deployment of an application to a scope "scope1" using environment in a different scope "scope2" #7895

Conversation

nithyatsu commented Sep 7, 2024

Description

Type of change

codecov bot commented Sep 7, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nithyatsu Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nithyatsu Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radius-functional-tests bot commented Sep 9, 2024 • edited Loading

Radius functional test overview

Test Status

radius-functional-tests bot commented Sep 9, 2024 • edited Loading

Radius functional test overview

Test Status

radius-functional-tests bot commented Sep 10, 2024 • edited Loading

Radius functional test overview

Test Status

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nithyatsu Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Sep 7, 2024 •

edited

Loading

nithyatsu Sep 10, 2024 •

edited

Loading

nithyatsu Sep 9, 2024 •

edited

Loading

radius-functional-tests bot commented Sep 9, 2024 •

edited

Loading

radius-functional-tests bot commented Sep 9, 2024 •

edited

Loading

radius-functional-tests bot commented Sep 10, 2024 •

edited

Loading

nithyatsu Sep 10, 2024 •

edited

Loading