Add UUID validation to rad env update
#5758
Conversation
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Test Results2 629 tests +1 2 620 ✔️ +1 2m 0s ⏱️ -1s Results for commit 676081e. ± Comparison against base commit 3fd59a0. This pull request removes 2 and adds 3 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
|
For the detailed report, please go to
|
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
|
For the detailed report, please go to
|
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
|
For the detailed report, please go to
|
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
|
For the detailed report, please go to
|
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
|
For the detailed report, please go to
|
pkg/cli/clivalidation.go
Outdated
| return "", err | ||
| } | ||
|
|
||
| if subscriptionId != "" { |
There was a problem hiding this comment.
If subscriptionId is empty, then we need to return error, don't we? I think uuid.Parse can returns error since subscriptionId is empty. So we do not need this if
There was a problem hiding this comment.
Yeah originally I implemented it that way but it caused an issue when the flag was not set: https://github.com/project-radius/radius/actions/runs/5315965610/jobs/9625032925
I have an idea on how to better handle this though, one sec
There was a problem hiding this comment.
Pushed an update so validation of the subscriptionId format only occurs if the flags are explicitly set, so it doesn't validate when they're omitted
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
|
For the detailed report, please go to
|
# Description Today we accept any string for `--azure-subscription-id`, which can be confusing if a user tries to use a subscription name instead of a subscription ID. We also don't have further downstream checks so this results in a really bad error for users when they do this. This PR adds CLI validation to make sure the supplied value is a valid UUID (_but doesn't check if it's a valid Azure subscription, that'll come later_). There is still work to do to add additional validation, which will come as part of a future PR ## Issue reference <!-- We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation. --> Partially addresses #5757 ## Checklist Please make sure you've completed the relevant tasks for this PR, out of the following list: * [x] Code compiles correctly * [x] Adds necessary unit tests for change * [x] Adds necessary E2E tests for change * [x] Unit tests passing * [x] Extended the documentation / Created issue for it ## Auto-generated summary <!-- GitHub Copilot for docs will auto-generate a summary of the PR --> <!-- copilot:all --> ### <samp>🤖 Generated by Copilot at 6876cc0</samp> ### Summary 🔄🆕✅ <!-- 1. 🔄 - This emoji represents the change in the `Validate` function to use the `RequireAzureSubscriptionId` function instead of directly getting the flag value. 2. 🆕 - This emoji represents the addition of the new function `RequireAzureSubscriptionId` to the `cli` package to validate Azure subscription ID flags for commands. 3. ✅ - This emoji represents the addition and update of test cases for Azure subscription ID validation in `update_test`. --> Added a new function to validate Azure subscription ID flags for `env update` and other commands. Updated tests to cover the validation logic. > _Sing, O Muse, of the skillful code review_ > _That brought new order to the update command_ > _And made it heed the azure flag, the true_ > _And valid sign of the subscription's hand._ ### Walkthrough * Import and use `uuid.Parse` and `AzureSubscriptionIdFlag` to validate Azure subscription ID flag value in `RequireAzureSubscriptionId` function ([link](https://github.com/project-radius/radius/pull/5758/files?diff=unified&w=0#diff-bf4e917446c73565ee6967f28bdd8e619c0d4fa40b215f195b840c021dd36ce9L25-R28), [link](https://github.com/project-radius/radius/pull/5758/files?diff=unified&w=0#diff-bf4e917446c73565ee6967f28bdd8e619c0d4fa40b215f195b840c021dd36ce9R264-R278)) * Use `RequireAzureSubscriptionId` function in `Validate` function of `update` command to ensure valid Azure subscription ID before updating environment ([link](https://github.com/project-radius/radius/pull/5758/files?diff=unified&w=0#diff-5666f913c4913108ef57c2f23dec3f4b6a4fc6bc653f2b0c51982b9619fb6e12L140-R140)) * Add test case to check error when Azure subscription ID flag value is invalid in `Test_Validate` function of `update_test` file ([link](https://github.com/project-radius/radius/pull/5758/files?diff=unified&w=0#diff-c8b9a3315a934ca0d8e0b750b2872603b1c504239f5d310251e87e5fb6a8e4a2L68-R78)) * Modify existing test case to use valid Azure subscription ID flag value in `Test_Validate` function of `update_test` file ([link](https://github.com/project-radius/radius/pull/5758/files?diff=unified&w=0#diff-c8b9a3315a934ca0d8e0b750b2872603b1c504239f5d310251e87e5fb6a8e4a2L78-R87))
Description
Today we accept any string for
--azure-subscription-id, which can be confusing if a user tries to use a subscription name instead of a subscription ID. We also don't have further downstream checks so this results in a really bad error for users when they do this.This PR adds CLI validation to make sure the supplied value is a valid UUID (but doesn't check if it's a valid Azure subscription, that'll come later).
There is still work to do to add additional validation, which will come as part of a future PR
Issue reference
Partially addresses #6302
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list:
Auto-generated summary
🤖 Generated by Copilot at 6876cc0
Summary
🔄🆕✅
Added a new function to validate Azure subscription ID flags for
env updateand other commands. Updated tests to cover the validation logic.Walkthrough
uuid.ParseandAzureSubscriptionIdFlagto validate Azure subscription ID flag value inRequireAzureSubscriptionIdfunction (link, link)RequireAzureSubscriptionIdfunction inValidatefunction ofupdatecommand to ensure valid Azure subscription ID before updating environment (link)Test_Validatefunction ofupdate_testfile (link)Test_Validatefunction ofupdate_testfile (link)