{AKS} az aks maintenanceconfiguration add/update: Add support for maintenanceWindow format in default maintenance configuration#33431
Conversation
anushkasingh16
requested review from
FumingZhang,
NoriZC,
teresaritorto and
yanzhudd
as code owners
️✔️AzureCLI-FullTest
|
|
Hi @anushkasingh16, |
️✔️AzureCLI-BreakingChangeTest
|
microsoft-github-policy-service
Bot
added
Auto-Assign
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Enable --schedule-type (Weekly-only) for the default AKS maintenance configuration using the maintenanceWindow format, and update CLI help/docs and tests accordingly.
Changes:
- Allow
--schedule-type Weeklyfor default maintenance configuration and constructmaintenance_windowpayload. - Add negative tests validating default-config argument constraints for
--schedule-typeand--interval-weeks. - Update parameter/help text and add CLI examples for default maintenanceWindow schedules.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/azure-cli/azure/cli/command_modules/acs/maintenanceconfiguration.py | Adds schedule_type handling for default config and builds maintenance_window model |
| src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_maintenanceconfiguration.py | Updates/extends tests for default config schedule-type validation |
| src/azure-cli/azure/cli/command_modules/acs/_params.py | Clarifies --schedule-type help now that default supports Weekly |
| src/azure-cli/azure/cli/command_modules/acs/_help.py | Adds examples and updates help text to mention default Weekly-only support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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>
|
|
nit: validation gap — for default config with also the example text says "in UTC every week" but the maintenanceWindow path supports |
|
Please fix the failed CI check Azure.azure-cli (Check the Format of Pull Request Title and Content), @anushkasingh16 |
anushkasingh16
changed the title
az aks maintenanceconfiguration add/update: Add support for maintenanceWindow format in default maintenance configuration
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
prior threads addressed:
No vote. |
Code reviewWhat this PR does (as built)Adds a Suggested change in directionThis PR ships in a new CLI major version, so we have a one-time opportunity to stop emitting
Either way, the result is: after this CLI version ships, no CLI invocation can produce a Note this is a breaking change in Issues in the current code1. Misleading error when 2. Mutation of caller's dict (low). raw_parameters["interval_weeks"] = 1mutates the dict the caller owns. Today nothing downstream re-reads it, but the function is presented as a pure "construct" function. Cleaner: pass 3. Inconsistent exception types for similar rejections (low). 4. Bulk error doesn't identify the offending flag (low). if any(raw_parameters.get(p) is not None for p in ("interval_days", "interval_months", "day_of_month", "week_index")):
raise MutuallyExclusiveArgumentError('--interval-days, --interval-months, --day-of-month and --week-index cannot be used ...')User gets a kitchen-sink message. Trivial improvement: offending = [p for p in (...) if raw_parameters.get(p) is not None]
if offending:
flags = ", ".join("--" + p.replace("_", "-") for p in offending)
raise MutuallyExclusiveArgumentError(f'{flags} cannot be used for default maintenance configuration.')5. Cosmetic rename adds noise (low). 6. Help text length (nit). 7. DRY in tests (low). 8. Missing test coverage (medium). 9. No test for Looks fine
|
Related issues worth referencingPR has no
If the team is tracking the work internally in ADO instead, please link the ADO item in the PR description so reviewers can find the design context. Pre-existing bug not introduced by this PR but worth flaggingThe legacy path's error message in
… is now wrong in this PR (it doesn't mention the new |
| self.assertEqual(result.maintenance_window.duration_hours, 4) | ||
| self.assertIsNotNone(result.maintenance_window.schedule) | ||
| self.assertIsNotNone(result.maintenance_window.schedule.weekly) | ||
| self.assertIsNone(getattr(result, 'time_in_week', None)) |
There was a problem hiding this comment.
Missing negative-test coverage: --day-of-week omitted.
There's no test that pins the user-facing error when --schedule-type Weekly is used for default config without --day-of-week. Today this falls through to constructWeeklySchedule's message (related: comment on maintenanceconfiguration.py:71). Once that fix lands, please add a test like:
def test_add_default_maintenance_configuration_requires_day_of_week(self):
# --schedule-type Weekly + start_time + duration, but no day_of_week
raw_parameters = { ..., "day_of_week": None, "start_time": "09:00", "duration_hours": 4 }
with self.assertRaises(RequiredArgumentMissingError):
aks_maintenanceconfiguration_update_internal(cmd, None, raw_parameters)This locks the message so a future refactor doesn't silently regress to the confusing generic weekly-schedule error.
There was a problem hiding this comment.
Added the test for this!
| self.assertEqual(result.maintenance_window.duration_hours, 4) | ||
| self.assertIsNotNone(result.maintenance_window.schedule) | ||
| self.assertIsNotNone(result.maintenance_window.schedule.weekly) | ||
| self.assertIsNone(getattr(result, 'time_in_week', None)) |
There was a problem hiding this comment.
No test exercises --utc-offset / --start-date on default config.
The comment at maintenanceconfiguration.py:78-79 makes a load-bearing promise — "the RP accepts the full MaintenanceWindow schema (including these optional fields)" — but the happy-path test passes utc_offset=None and start_date=None, so a future model regen that quietly drops one of those fields would not be caught here.
Suggest extending this test (or adding a sibling) that sets:
"utc_offset": "+05:30",
"start_date": "2026-01-15",and asserts both land on the constructed result.maintenance_window (utc_offset, start_date).
yanzhudd
changed the title
az aks maintenanceconfiguration add/update: Add support for maintenanceWindow format in default maintenance configurationaz aks maintenanceconfiguration add/update: Add support for maintenanceWindow format in default maintenance configuration
Re-review (iter 3 / commit a2521bc)Closed cleanly on this push:
Still open — one carryover from the prior round:
Otherwise LGTM. Not approving — leaving that to the AKS-side maintainers per area ownership. |
Re-review (iter 4 / commit 50281c3)Carryover from prior pass closed cleanly:
All asks across both review rounds are now closed. LGTM, no further comments. Not approving — leaving that to the AKS-side maintainers. No vote. |
yewmsft
left a comment
yewmsft
left a comment
There was a problem hiding this comment.
All 6 review comments verified at HEAD 50281c3:
--day-of-weekvalidation added with negative test ✓raw_parametersmutation removed (uses{**raw_parameters, "interval_weeks": 1}) ✓- Offending-flag enumeration in the inapplicable-params error + tests ✓
- Help short-summaries shortened ✓
- Negative test for missing
--day-of-week✓ - Test for
--utc-offset/--start-dateround-trip on default config ✓
nit (non-blocking): --start-date short-summary at _help.py:1499 and :1637 has a stray trailing " ("...including default.""). worth a one-line follow-up.
Related command
az aks maintenanceconfiguration add
az aks maintenanceconfiguration update
Description
Previously, the default maintenance configuration only supported the legacy timeInWeek format (using --weekday and --start-hour).
The AKS RP API (since 2023-05-01) also supports the maintenanceWindow format for the default config, but the CLI was blocking it by raising an error when --schedule-type was provided for the default config.
This PR removes that restriction and allows users to use --schedule-type Weekly with the default maintenance configuration name, which routes to the maintenanceWindow format. The RP enforces that only Weekly schedule with intervalWeeks=1 is valid for the default config (since API version 2025-03-01), so the CLI validates this client-side as well.
The legacy --weekday --start-hour path continues to work unchanged for backward compatibility.
Testing Guide
History Notes
[AKS]
az aks maintenanceconfiguration add/update: Add support for maintenanceWindow format in default maintenance configurationThis 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.