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

Fix Delete application confirmation message points to the workspace but says environment #7089 #7095

Merged
merged 3 commits into from
Feb 1, 2024

Conversation

jhandel
Copy link
Contributor

@jhandel jhandel commented Jan 31, 2024

Signed-off-by: Josh josh@liveoak.ws

Description

Parse out the environment name from the Workspace.Environment property using the resources library.

Type of change

This pull request fixes a bug in Radius and has an approved issue

Fixes: #7089

@AaronCrawfis
Copy link
Contributor

Thanks for the contribution @jhandel! For #7089, it's indeed a bug where it says "environment" when the value printed is the workspace name. Although, instead of updating the label I think it might be better to update the value to the environment name. We're trying to make workspaces an advanced concept that only need to be referenced/used if you need to opt-into the complexity of managing multiple environments/installations of Radius from a single machine. For the 99% case we want applications/environments to be the main resources and concepts users interact with.

I think a great solution to the bug in #7089 would be to update https://github.com/radius-project/radius/blob/main/pkg/cli/cmd/app/delete/delete.go#L146 to use the environment name instead of the workspace name, and keep the string as-is.

Thanks again for taking a crack at this! We do bi-weekly triage and you caught the issue before we could take a look at possible fixes and post a reply. Very much appreciate the pro-active fixes!

@jhandel
Copy link
Contributor Author

jhandel commented Feb 1, 2024

Thanks for the contribution @jhandel! For #7089, it's indeed a bug where it says "environment" when the value printed is the workspace name. Although, instead of updating the label I think it might be better to update the value to the environment name. We're trying to make workspaces an advanced concept that only need to be referenced/used if you need to opt-into the complexity of managing multiple environments/installations of Radius from a single machine. For the 99% case we want applications/environments to be the main resources and concepts users interact with.

I think a great solution to the bug in #7089 would be to update https://github.com/radius-project/radius/blob/main/pkg/cli/cmd/app/delete/delete.go#L146 to use the environment name instead of the workspace name, and keep the string as-is.

Thanks again for taking a crack at this! We do bi-weekly triage and you caught the issue before we could take a look at possible fixes and post a reply. Very much appreciate the pro-active fixes!

Thanks for the feedback.. I have parsed out the environment name from the Workspace.Environment property using the resource parsing lib.

Though this brings up the question on if we should be adding env as an optional attribute to the delete command like resource group is. Just food for thought.

(also eventually I will figure out how to sign my commits properly from inside of VS code without having to do the rebase and force commit trick after messing it up lol... )

…ce but says environment radius-project#7089

Signed-off-by: Josh Handel <josh@liveoak.ws>
Signed-off-by: Josh <josh@liveoak.ws>
Signed-off-by: Josh josh@liveoak.ws
Signed-off-by: Josh <josh@liveoak.ws>
thanks for catching the typo.

Co-authored-by: Shalabh Mohan Shrivastava <shalabhms@gmail.com>
Signed-off-by: Josh <josh@liveoak.ws>
@shalabhms
Copy link
Contributor

@jhandel - thanks for making the contribution . Could you please update the description which is pointing to your earlier commit changes?

@jhandel
Copy link
Contributor Author

jhandel commented Feb 1, 2024

@jhandel - thanks for making the contribution . Could you please update the description which is pointing to your earlier commit changes?

done.... and thank you for the review of the PR.

@shalabhms shalabhms changed the title fix for Delete application confirmation message points to the workspace but says environment #7089 Fix Delete application confirmation message points to the workspace but says environment #7089 Feb 1, 2024
@shalabhms shalabhms added triaged This issue has been reviewed and triaged and removed triaged This issue has been reviewed and triaged labels Feb 1, 2024
@shalabhms shalabhms self-requested a review February 1, 2024 20:56
@radius-functional-tests
Copy link

radius-functional-tests bot commented Feb 1, 2024

Radius functional test overview

🔍 Go to test action run

Name Value
Repository jhandel/radius
Commit ref 0f4e13f
Unique ID 3b4aadfb40
Image tag pr-3b4aadfb40
Click here to see the list of tools in the current test run
  • gotestsum 1.10.0
  • KinD: v0.20.0
  • Dapr: 1.12.0
  • Azure KeyVault CSI driver: 1.4.2
  • Azure Workload identity webhook: 1.1.0
  • Bicep recipe location ghcr.io/radius-project/dev/test/functional/shared/recipes/<name>:pr-3b4aadfb40
  • 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-3b4aadfb40
  • controller test image location: ghcr.io/radius-project/dev/controller:pr-3b4aadfb40
  • ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-3b4aadfb40
  • 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 samples functional tests...
⌛ Starting shared functional tests...
⌛ Starting datastoresrp functional tests...
⌛ Starting daprrp functional tests...
⌛ Starting kubernetes functional tests...
⌛ Starting msgrp functional tests...
⌛ Starting ucp functional tests...
✅ samples functional tests succeeded
✅ kubernetes functional tests succeeded
✅ msgrp functional tests succeeded
✅ datastoresrp functional tests succeeded
✅ ucp functional tests succeeded
✅ daprrp functional tests succeeded
✅ shared functional tests succeeded

Copy link
Contributor

@ytimocin ytimocin left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @jhandel !

@ytimocin ytimocin merged commit c1a8d16 into radius-project:main Feb 1, 2024
16 checks passed
@jhandel jhandel deleted the fix_7089 branch February 2, 2024 12:06
willdavsmith pushed a commit to willdavsmith/radius that referenced this pull request Mar 4, 2024
…ut says environment radius-project#7089 (radius-project#7095)

Signed-off-by: Josh <josh@liveoak.ws>

# Description

Parse out the environment name from the Workspace.Environment property
using the resources library.

## Type of change

This pull request fixes a bug in Radius and has an approved issue

Fixes: radius-project#7089

---------

Signed-off-by: Josh Handel <josh@liveoak.ws>
Signed-off-by: Josh <josh@liveoak.ws>
Signed-off-by: Josh josh@liveoak.ws
Co-authored-by: Shalabh Mohan Shrivastava <shalabhms@gmail.com>
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.

Delete application confirmation message points to the workspace but says environment
4 participants