-
Notifications
You must be signed in to change notification settings - Fork 3.3k
{Core} Adopt serialization func from azure-core for CLI's todict formatting
#31775
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
Conversation
️✔️AzureCLI-FullTest
|
️✔️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>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the CLI’s todict utility to use Azure Core’s serialization helpers (is_generated_model and attribute_list) instead of the older as_dict logic, ensuring correct handling of renamed/ formatted SDK model attributes.
- Import
is_generated_modelandattribute_listfromazure.core.serialization - Replace the old
as_dictbranch with a single path usingis_generated_model - Remove legacy typespec
as_dicthandling
Comments suppressed due to low confidence (2)
src/azure-cli-core/azure/cli/core/util.py:654
- Add unit tests for this new
is_generated_modelbranch to cover both swagger and typespec generated models, verifying thatattribute_listcorrectly enumerates renamed/formatted attributes.
if is_generated_model(obj):
src/azure-cli-core/azure/cli/core/util.py:629
- Update the function docstring to mention the support for Azure Core generated models via
is_generated_modelandattribute_list.
def todict(obj, post_processor=None):
todict formatting
| if is_generated_model(obj): | ||
| result = {} | ||
| for attr in attribute_list(obj): | ||
| if hasattr(obj, attr): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to check whether the attr exist because we may import SDK model and then do some customization like keyvault certificate L122-127
azure-cli/src/azure-cli/azure/cli/command_modules/keyvault/custom.py
Lines 69 to 128 in bb2b31a
| def _default_certificate_profile(cmd): | |
| def get_model(x): | |
| return cmd.loader.get_sdk(x, resource_type=ResourceType.DATA_KEYVAULT_CERTIFICATES, mod='_generated_models') | |
| Action = get_model('Action') | |
| ActionType = get_model('ActionType') | |
| KeyUsageType = get_model('KeyUsageType') | |
| CertificateAttributes = get_model('CertificateAttributes') | |
| CertificatePolicy = get_model('CertificatePolicy') | |
| IssuerParameters = get_model('IssuerParameters') | |
| KeyProperties = get_model('KeyProperties') | |
| LifetimeAction = get_model('LifetimeAction') | |
| SecretProperties = get_model('SecretProperties') | |
| X509CertificateProperties = get_model('X509CertificateProperties') | |
| Trigger = get_model('Trigger') | |
| template = CertificatePolicy( | |
| key_properties=KeyProperties( | |
| exportable=True, | |
| key_type='RSA', | |
| key_size=2048, | |
| reuse_key=True | |
| ), | |
| secret_properties=SecretProperties( | |
| content_type='application/x-pkcs12' | |
| ), | |
| x509_certificate_properties=X509CertificateProperties( | |
| key_usage=[ | |
| KeyUsageType.c_rl_sign, | |
| KeyUsageType.data_encipherment, | |
| KeyUsageType.digital_signature, | |
| KeyUsageType.key_encipherment, | |
| KeyUsageType.key_agreement, | |
| KeyUsageType.key_cert_sign | |
| ], | |
| subject='CN=CLIGetDefaultPolicy', | |
| validity_in_months=12 | |
| ), | |
| lifetime_actions=[LifetimeAction( | |
| trigger=Trigger( | |
| days_before_expiry=90 | |
| ), | |
| action=Action( | |
| action_type=ActionType.auto_renew | |
| ) | |
| )], | |
| issuer_parameters=IssuerParameters( | |
| name='Self', | |
| ), | |
| attributes=CertificateAttributes( | |
| enabled=True | |
| ) | |
| ) | |
| del template.id | |
| del template.attributes | |
| del template.issuer_parameters.certificate_type | |
| del template.lifetime_actions[0].trigger.lifetime_percentage | |
| del template.x509_certificate_properties.subject_alternative_names | |
| del template.x509_certificate_properties.ekus | |
| return template |
Some attributes are deleted
| # The way to detect if it's a typespec generated model is to check the private `_is_model` attribute | ||
| # azure-core provided new function `attribute_list` to list all attribute names | ||
| # so that we don't need to use raw __dict__ directly | ||
| if getattr(obj, "_is_model", False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did it handle the nested case? When the internal property is also an object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will call todict recursively
Related command
All cli commands
Description
Previously we added dict transformation for typespec generated SDK models in #30339, we called typespec generated SDK model's
as_dictfunc to get each attribute name and attribute value. But this still causes issues like #31529 reported whenWe can take the
expiredattribute from Keyvault SDK as an example:The
expiresis renamed fromexpin http response and hasunix-timestamptime format. By callingas_dict{'expires': '2027-05-28T16:43:07+00:00'}(from swagger generated SDK model){'exp': 1811522587}(from typespec generated SDK model).The renaming and formatting was not applied to
as_dictand this behavior applies for all typespec generated SDK models.To resolve this breaking change, SDK team added related serialization support in azure-core with Azure/azure-sdk-for-python#41445, Azure/azure-sdk-for-python#41571. They now have two functions to handle both swagger generated SDK and typespec generated SDK:
is_generated_modelto identify auto generated SDK modelsattribute_listto list all attribute names which are after renaminggetattr(obj, attr)for the attribute we get fromattribute_listto get the attribute value which is after formattingThis PR changes the dict formatting for typespec generated SDK models with new
attribute_listfunc. We didn't adoptis_generated_modelbecause we don't want to change swagger generated SDK models' dict formatting to avoid any regressions.Testing Guide
History Notes
[Component Name 1] BREAKING CHANGE:
az command a: Make some customer-facing breaking change[Component Name 2]
az command b: Add some customer-facing featureThis 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.