Azure: Fix modular push #159
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit fixes the Azure's modular push by adding to the missing `ProductProperty` model to the submission request, during the building of the filtered resources on `get_modular_resources_to_publish` After re-checking the documentation and performing a manual testing it was noticed that this was the last missing piece for the modular push. Refers to SPSTRAT-633 Signed-off-by: Jonathan Gangi <[email protected]>
- The `terms_of_use` comes as `termsOfUseUrl` from JSON instead of `termsOfUse` - For whatever reason Microsoft is sending back this property with the legacy schema URL, namely "product-ingestion.azureedge.net" instead of "schema.mp.microsoft.com", so we need to force the correct value. Refers to SPSTRAT-633 Signed-off-by: Jonathan Gangi <[email protected]>
Reviewer's GuideThis PR fixes Azure modular push by including the ProductProperty resource and correcting its schema and terms_of_use mapping, and updates tests to validate the new behavior. Sequence diagram for Azure modular push resource selection including ProductPropertysequenceDiagram
actor Operator
participant AzureService
participant AzureAPI
Operator->>AzureService: get_modular_resources_to_publish(tech_config)
activate AzureService
AzureService->>AzureService: product_id = tech_config.product_id
AzureService->>AzureService: plan_id = tech_config.plan_id
AzureService->>AzureService: filter_product_resources(product, product)
AzureService->>AzureService: select ProductSummary with matching product_id
AzureService-->>AzureService: prod_res
AzureService->>AzureService: filter_product_resources(product, property)
AzureService->>AzureService: select ProductProperty with matching product_id
AzureService-->>AzureService: property
AzureService->>AzureService: filter_plan_resources(product, plan)
AzureService->>AzureService: select PlanSummary with matching plan_id
AzureService-->>AzureService: plan_res
AzureService-->>Operator: [prod_res, property, plan_res, tech_config]
deactivate AzureService
Note over AzureService,AzureAPI: Updated to include ProductProperty in modular push resources
Updated class diagram for Azure ProductProperty modelclassDiagram
class AzureProductLinkedResource {
}
class ProductProperty {
+str schema
+str kind
+str terms_of_use
}
AzureProductLinkedResource <|-- ProductProperty
%% Field details
class ProductProperty {
+str schema "alias: MS_SCHEMA, const: https://schema.mp.microsoft.com/schema/property/2022-03-01-preview2"
+str kind "expected value: azureVM"
+str terms_of_use "optional, alias: termsOfUseUrl"
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In get_modular_resources_to_publish, avoid using
propertyas a local variable name since it shadows the built-in and can be confusing; consider renaming it to something likeproduct_property. - When selecting
prod_res,property, andplan_resvia list comprehensions and[0], consider handling the case where no matching resource is found to avoid an unhelpful IndexError and provide a clearer failure mode.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In get_modular_resources_to_publish, avoid using `property` as a local variable name since it shadows the built-in and can be confusing; consider renaming it to something like `product_property`.
- When selecting `prod_res`, `property`, and `plan_res` via list comprehensions and `[0]`, consider handling the case where no matching resource is found to avoid an unhelpful IndexError and provide a clearer failure mode.
## Individual Comments
### Comment 1
<location> `cloudpub/ms_azure/service.py:583-592` </location>
<code_context>
if prd.id == product_id
],
)[0]
+ property = cast(
+ List[ProductProperty],
+ [
</code_context>
<issue_to_address>
**suggestion:** Avoid shadowing the built-in `property` when naming this variable.
Renaming this variable (e.g., to `property_res` or `product_property`) will avoid confusion with the built-in and make the code clearer. Please update the name and use it consistently in the return list.
Suggested implementation:
```python
product_property = cast(
List[ProductProperty],
[
prop
for prop in self.filter_product_resources(product=product, resource="property")
if prop.product_id == product_id
],
)[0]
```
To fully apply the rename and avoid shadowing the built-in `property`, you should:
1) Update all subsequent references to this variable within the same function to use `product_property` instead of `property`. This likely includes its use in the return value or any intermediate processing.
2) If this value is unpacked into a response/DTO, ensure parameter names passed into constructors or dicts are still correct (only the local variable name should change, not external API/serialization field names).
</issue_to_address>
### Comment 2
<location> `cloudpub/ms_azure/service.py:583-590` </location>
<code_context>
def get_modular_resources_to_publish(
self, product: Product, tech_config: VMIPlanTechConfig
) -> List[AzureResource]:
"""Return the required resources for a modular publishing.
According to Microsoft docs:
"For a modular publish, all resources are required except for the product level details
(for example, listing, availability, packages, reseller) as applicable to your
product type."
Args:
product (Product): The original product to filter the resources from
tech_config (VMIPlanTechConfig): The updated tech config to publish
Returns:
List[AzureResource]: _description_
"""
# The following resources shouldn't be required:
# -> customer-leads
# -> test-drive
# -> *listing*
# -> reseller
# -> price-and-availability-*
# NOTE: The "submission" resource will be already added by the "submit_to_status" method
#
# With that it needs only the related "product", "property" and "plan" resources alongisde
# the updated tech_config
product_id = tech_config.product_id
plan_id = tech_config.plan_id
prod_res = cast(
List[ProductSummary],
[
prd
for prd in self.filter_product_resources(product=product, resource="product")
if prd.id == product_id
],
)[0]
property = cast(
List[ProductProperty],
[
prop
for prop in self.filter_product_resources(product=product, resource="property")
if prop.product_id == product_id
],
)[0]
plan_res = cast(
List[PlanSummary],
[
pln
for pln in self.filter_product_resources(product=product, resource="plan")
if pln.id == plan_id
],
)[0]
return [prod_res, property, plan_res, tech_config]
</code_context>
<issue_to_address>
**issue (code-quality):** Don't assign to builtin variable `property` ([`avoid-builtin-shadow`](https://docs.sourcery.ai/Reference/Default-Rules/comments/avoid-builtin-shadow/))
<br/><details><summary>Explanation</summary>Python has a number of `builtin` variables: functions and constants that
form a part of the language, such as `list`, `getattr`, and `type`
(See https://docs.python.org/3/library/functions.html).
It is valid, in the language, to re-bind such variables:
```python
list = [1, 2, 3]
```
However, this is considered poor practice.
- It will confuse other developers.
- It will confuse syntax highlighters and linters.
- It means you can no longer use that builtin for its original purpose.
How can you solve this?
Rename the variable something more specific, such as `integers`.
In a pinch, `my_list` and similar names are colloquially-recognized
placeholders.</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Ignore the fact that one of the elements of the union doesn't have `product_id`, as `ProductProperty` has it. Refers to SPSTRAT-633 Signed-off-by: Jonathan Gangi <[email protected]>
Collaborator
Author
lslebodn
reviewed
Nov 26, 2025
| ], | ||
| )[0] | ||
| return [prod_res, plan_res, tech_config] | ||
| return [prod_res, property, plan_res, tech_config] |
Collaborator
There was a problem hiding this comment.
a bit off-topic question.
Does azure API care about order of resources in the list?
Collaborator
Author
There was a problem hiding this comment.
No, it doesn't. But I've added in this particular order because the property is a Product linked resource, not a plan linked one, so it would be less confusing in the code, I guess
Collaborator
Author
There was a problem hiding this comment.
tech_config for example is a product and plan linked resource 🙂
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes the Azure's modular push by adding to the missing
ProductPropertymodel to the submission request, during the building of the filtered resources onget_modular_resources_to_publishAfter re-checking the documentation and performing a manual testing it was noticed that this was the last missing piece for the modular push.
It also fixes the following detected issues with the
ProductProperty:The
terms_of_usecomes astermsOfUseUrlfrom JSON instead oftermsOfUseFor whatever reason Microsoft is sending back this property with the
legacy schema URL, namely "product-ingestion.azureedge.net" instead of
"schema.mp.microsoft.com", so we need to force the correct value.
Refers to SPSTRAT-633
Signed-off-by: Jonathan Gangi [email protected]
Summary by Sourcery
Fix Azure modular publishing by including product property resources and aligning the ProductProperty model with the latest schema and field naming.
Bug Fixes:
Tests: