[containerapp] az containerapp env workload-profile -n <name> -g <resourceGroup> --workload-profile-type flex Simplify flex wp creation
#32713
Conversation
jepetty
requested review from
howang-ms,
yanzhudd and
zhoxing-ms
as code owners
️✔️AzureCLI-FullTest
|
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| containerapp env workload-profile add | cmd containerapp env workload-profile add update parameter workload_profile_name: removed property required=True |
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
microsoft-github-policy-service
bot
requested review from
JennyLawrance,
chinadragon0515,
ebencarek,
jijohn14,
jsntcy,
pagariyaalok,
ruslany,
sanchitmehta,
torosent,
vinisoto,
vturecek,
wangzelin007 and
yonzhan
There was a problem hiding this comment.
Pull request overview
This pull request simplifies the creation of flex workload profiles in Azure Container Apps by making the --workload-profile-name parameter optional when the profile type is "flex". The API automatically assigns a default name for flex profiles, removing the need for users to provide one manually.
Changes:
- Made
workload_profile_nameparameter optional in theadd_workload_profilefunction - Added validation to require
--workload-profile-nameonly for non-flex workload profile types - Updated workload profile setup logic to handle None values for profile names
- Added comprehensive test case to verify flex profiles can be created without names and non-flex profiles still require names
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| custom.py | Made workload_profile_name parameter optional with default value None, updated duplicate name check to handle None |
| containerapp_env_decorator.py | Added validation requiring profile names only for non-flex types, updated profile matching logic to handle None names |
| test_containerapp_workload_profile_commands.py | Added test verifying flex profiles work without names and non-flex profiles fail without names |
Comments suppressed due to low confidence (1)
src/azure-cli/azure/cli/command_modules/containerapp/containerapp_env_decorator.py:311
- The comment says "flex profiles will be named automatically" but this is an implementation detail of the API, not the CLI. Consider clarifying that the API (not the CLI) assigns the default name, e.g., "workload-profile-name is optional for flex profiles as the API will assign a default name automatically".
# validate workload profile arguments - flex profiles will be named automatically
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| workload_profile_type = workload_profile_type.upper() | ||
| workload_profiles = r["properties"]["workloadProfiles"] | ||
| profile = [p for p in workload_profiles if p["name"].lower() == workload_profile_name.lower()] | ||
| profile = [p for p in workload_profiles if workload_profile_name and p["name"].lower() == workload_profile_name.lower()] |
There was a problem hiding this comment.
When workload_profile_name is None (for flex profiles), the short-circuit evaluation in the list comprehension ensures that profile will always be an empty list, causing the code to create a new profile at line 389 with {"name": None}. This will serialize to {"name": null} in JSON and be sent to the API. While the test at line 468 suggests the API handles this, it would be more robust to conditionally exclude the "name" key when it's None rather than sending null. Consider changing line 389 to: profile = {} if workload_profile_name is None else {"name": workload_profile_name}, and then conditionally add the name key only when needed.
|
|
||
| @AllowLargeResponse(8192) | ||
| @ResourceGroupPreparer(location="northcentralus") | ||
| def test_containerapp_env_workload_profiles_name_validation(self, resource_group): |
There was a problem hiding this comment.
The test function name "test_containerapp_env_workload_profiles_name_validation" doesn't accurately describe what the test does. It's not just testing name validation - it's testing that flex profiles can be created without a name, and that non-flex profiles require a name. Consider renaming to something like "test_containerapp_env_workload_profiles_flex_name_optional" or "test_containerapp_env_workload_profiles_name_requirement" to better reflect the actual test behavior.
| def test_containerapp_env_workload_profiles_name_validation(self, resource_group): | |
| def test_containerapp_env_workload_profiles_flex_name_optional(self, resource_group): |
| workload_profile_type = self.get_argument_workload_profile_type() | ||
|
|
||
| if workload_profile_name: | ||
| # We have already validated that the configuration is valid if adding a workload profile |
There was a problem hiding this comment.
The comment "We have already validated that the configuration is valid if adding a workload profile" is placed at the start of the conditional block, but the validation happens earlier in validate_arguments(). Consider moving or rephrasing this comment to clarify that it's referring to validation that occurred in validate_arguments(), not within this method. Something like "Validation in validate_arguments() ensures this configuration is valid" would be clearer.
| # We have already validated that the configuration is valid if adding a workload profile | |
| # Validation in validate_arguments() ensures this configuration is valid when adding a workload profile |
| @ResourceGroupPreparer(location="northcentralus") | ||
| def test_containerapp_env_workload_profiles_name_validation(self, resource_group): | ||
| # Set location to a region that supports flex workload profiles | ||
| TEST_LOCATION = "northcentralusstage" |
There was a problem hiding this comment.
The test uses "northcentralusstage" as the location for testing flex workload profiles. This appears to be a staging environment. Consider documenting whether this test will work in production environments, or if it requires special access to staging. If this is intended only for internal testing, consider adding a skip decorator or condition check.
| if workload_profile_name: | ||
| # We have already validated that the configuration is valid if adding a workload profile | ||
| if workload_profile_name or workload_profile_type: | ||
| if "workloadProfiles" not in r["properties"] or not r["properties"]["workloadProfiles"]: |
There was a problem hiding this comment.
Please a test to cover this case.
|
Hi @jepetty Which command this PR for? I reviewed this PR, the change is related to command Please add more test case and description for the PR change. |
|
|
||
| if workload_profile_name: | ||
| # We have already validated that the configuration is valid if adding a workload profile | ||
| if workload_profile_name or workload_profile_type: |
There was a problem hiding this comment.
Does this change want support command only input --workload-profile-type without --workload-profile-name?
4 participants
Related command
Description
This simplifies CLI invocation when creating a new workload-profile for ACA with type flex - the API will automatically assign this profile with a default name, so users don't need to provide one themselves.
Testing Guide
This checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.