[PostgreSQL] az postgres flexible-server server-logs list: Fix 33090, command crashes with an AttributeError when listing log files#33096
Conversation
… removing created_time attributes
nachoalonsoportillo
requested review from
Pan-Qi,
VeryEarly and
calvinhzy
as code owners
️✔️AzureCLI-FullTest
|
|
Hi @nachoalonsoportillo, |
️✔️AzureCLI-BreakingChangeTest
|
|
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>
|
|
Fixes #33090. |
There was a problem hiding this comment.
Pull request overview
This PR updates the PostgreSQL flexible-server server logs listing implementation to sanitize captured log file objects before returning them, specifically to remove created_time-related fields that are causing SDK compatibility issues.
Changes:
- Added
_sanitize_log_file()helper to convert log file objects into dictionaries and recursively removecreated_timevariants. - Updated
flexible_server_list_log_files_with_filterandflexible_server_log_listto return sanitized log file dictionaries instead of mutating SDK objects.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/azure-cli/azure/cli/command_modules/postgresql/commands/server_logs_commands.py
Outdated
Show resolved
Hide resolved
src/azure-cli/azure/cli/command_modules/postgresql/commands/server_logs_commands.py
Outdated
Show resolved
Hide resolved
src/azure-cli/azure/cli/command_modules/postgresql/commands/server_logs_commands.py
Show resolved
Hide resolved
nachoalonsoportillo
changed the title
az postgres flexible-server server-logs list: BUG FIX, command crashes with an AttributeError when listing log files
…jects for SDK compatibility
…es for SDK compatibility
nachoalonsoportillo
modified the milestones:
May 2026 (2026-05-05),
April 2026 (2026-04-07)
…og file generation wait logic - Removed the conditional return for live tests. - Combined the environment variable check with the sleep function to streamline the waiting period for log file generation.
|
@calvinhzy and @yonzhan please trigger checks |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
nachoalonsoportillo
changed the title
az postgres flexible-server server-logs list: BUG FIX, command crashes with an AttributeError when listing log filesaz postgres flexible-server server-logs list: Fix 33090, command crashes with an AttributeError when listing log files
|
@calvinhzy and @yonzhan please trigger checks, fix we put in for the title, and we needed this for the April release |
nachoalonsoportillo
modified the milestones:
April 2026 (2026-04-07),
May 2026 (2026-05-05)
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # wait for around 30 min to allow log files to be generated | ||
| sleep(30*60) | ||
| os.environ.get(ENV_LIVE_TEST, False) and sleep(30*60) # wait for around 30 min to allow log files to be generated |
There was a problem hiding this comment.
os.environ.get(...) returns a string when the variable is set; values like 'false'/'0' are still truthy and will trigger the sleep unexpectedly. Prefer parsing the env var to a real boolean (or using the existing test framework helpers for live/playback detection) and use an explicit if instead of and for side effects.
| os.environ.get(ENV_LIVE_TEST, False) and sleep(30*60) # wait for around 30 min to allow log files to be generated | |
| live_test_flag = os.environ.get(ENV_LIVE_TEST, "") | |
| is_live_test = live_test_flag.lower() in ("1", "true", "yes", "y", "on") | |
| if is_live_test: | |
| sleep(30 * 60) # wait for around 30 min to allow log files to be generated |
|
|
||
| # wait for around 30 min to allow log files to be generated | ||
| sleep(30*60) | ||
| os.environ.get(ENV_LIVE_TEST, False) and sleep(30*60) # wait for around 30 min to allow log files to be generated |
There was a problem hiding this comment.
A fixed 30-minute sleep in live tests can significantly increase pipeline duration and may cause timeouts/flakiness. Consider polling server-logs list until at least one expected log appears (with a bounded timeout and short backoff), or reduce the wait and make the test resilient to empty results.
| # Add systemData if not present (for backward compatibility) | ||
| if 'systemData' not in result: | ||
| result['systemData'] = None |
There was a problem hiding this comment.
Adding systemData (even as None) changes the command output shape for consumers that were previously on the older SDK (where this field did not exist). If the intent is to preserve the legacy contract, it’s safer to omit systemData from the returned dict (and potentially strip it when present) rather than always introducing the key.
| # Add systemData if not present (for backward compatibility) | |
| if 'systemData' not in result: | |
| result['systemData'] = None | |
| # Remove systemData to preserve the legacy output contract | |
| result.pop('systemData', None) |
| if 'createdTime' in properties: | ||
| del properties['createdTime'] # Remove created_time as it's not in the original SDK model |
There was a problem hiding this comment.
The PR description proposes adding a deprecation note warning that the returned shape will change in the future to surface createdTime, but this change isn’t reflected in the code/help here. Either add the deprecation notice (e.g., command help text/release note) or remove that claim from the description to keep them aligned.
| properties = result.pop('properties') | ||
| # Extract type from properties and rename to typePropertiesType | ||
| if 'createdTime' in properties: | ||
| del properties['createdTime'] # Remove created_time as it's not in the original SDK model |
There was a problem hiding this comment.
The inline comment uses the snake_case name created_time, but the code is deleting the serialized key createdTime. Updating the comment to match the actual key being removed would prevent confusion during future SDK transitions.
| del properties['createdTime'] # Remove created_time as it's not in the original SDK model | |
| del properties['createdTime'] # Remove createdTime as it's not in the original SDK model |
Related command
Description
az postgres flexible-server server-logs listcrashes with an AttributeError when listing log files.Here are the significant differences between LogFile (old) and CapturedLog (new):
Field changes
Read-only enforcement
In the old SDK, created_time and last_modified_time were declared readonly: True in _validation — i.e., server-side only. In the new SDK, both are declared with full CRUD visibility (["read", "create", "update", "delete", "query"]), removing the hard read-only constraint at the SDK layer.
SDK generation style
The old SDK was AutoRest-generated (_serialization.Model, _validation, _attribute_map dicts). The new SDK uses the Python Code Generator (_Model, rest_field, @overload constructors). This changes how serialization and validation are expressed, though the wire format is the same.
For now, let's fix it like this. My proposal is to also add a deprecation note to inform that shape of returned object will change in the future to, at least, surface createdTime. We could also consider renaming typePropertiestype to something like logType.
Testing Guide
I noticed that the existing tests weren't running the `server-logs list' command when live test was run. That would have caught the error before we would have merged the PR that introduced the offending SDK. So, I've enable it and refreshed the recordings.
History Notes
[PostgreSQL]
az postgres flexible-server server-logs list: Fix 33090, command crashes with an AttributeError when listing log filesThis 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.