SPSTRAT-630: AzureProvider: Rework draft detection #129
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
Reviewer's GuideRefactor AzureProvider.ensure_offer_is_writable to replace the Borg-based draft tracking with a submissions-driven approach using updated cloudpub APIs, filter out technical VM resources before diffing, correct outdated method signatures, and align tests with the new logic. Sequence diagram for new draft detection in ensure_offer_is_writablesequenceDiagram
participant AzureProvider
participant AzurePublishService
participant "cloudpub API"
AzureProvider->>AzurePublishService: get_productid(offer_name)
AzurePublishService->>"cloudpub API": fetch product id
AzurePublishService-->>AzureProvider: product_id
AzureProvider->>AzurePublishService: get_submissions(product_id)
AzurePublishService->>"cloudpub API": fetch submissions
AzurePublishService-->>AzureProvider: submissions
AzureProvider->>AzurePublishService: get_product(product_id, last_state)
AzurePublishService->>"cloudpub API": fetch offer (preview/live)
AzurePublishService-->>AzureProvider: offer_latest
AzureProvider->>AzurePublishService: get_product(product_id, "draft")
AzurePublishService->>"cloudpub API": fetch offer (draft)
AzurePublishService-->>AzureProvider: offer_draft
AzureProvider->>AzurePublishService: diff_two_offers(offer_latest, offer_draft)
AzurePublishService-->>AzureProvider: diff
AzureProvider->>AzureProvider: filter_out_technical_resources
AzureProvider->>AzureProvider: raise error if diff exists
ER diagram for submissions-driven offer state detectionerDiagram
PRODUCT ||--o{ SUBMISSION : has
SUBMISSION }o--|| TARGET : targets
PRODUCT {
string id
string name
}
SUBMISSION {
string id
string result
TARGET target
}
TARGET {
string targetType
}
Class diagram for AzureProvider and removal of AzureDestinationBorgclassDiagram
class AzureProvider {
- publish_svc: AzurePublishService
+ ensure_offer_is_writable(destination: str, nochannel: bool)
+ _name_from_push_item(push_item: VHDPushItem) : str
}
class AzurePublishService {
+ get_productid(offer_name: str)
+ get_submissions(product_id: str)
+ get_product(product_id: str, state: str)
+ diff_two_offers(offer_latest, offer_draft)
}
AzureProvider --> AzurePublishService
class VMIPlanTechConfig
class CoreVMIPlanTechConfig
AzureProvider ..> VMIPlanTechConfig : uses for filtering
AzureProvider ..> CoreVMIPlanTechConfig : uses for filtering
%% AzureDestinationBorg was removed
%% No longer referenced by AzureProvider
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Collaborator
Author
|
@lslebodn it's still a draft and I need to update the tests yet, but can you please check whether it makes sense? The idea here is that we only "prohibit" pushing to an offer whether it has been modified by cloudops hence I'm performing a diff between preview/live and draft while ignoring any change on TechnicalConfig (as it might be a pre-push change). With that we also get rid of the Borg since we consider any change on TechnicalConfig is ours and keep the code cleaner. Please let me know your thoughts, I'll fix the tests later on |
This commit reworks the `AzureProvider.ensure_offer_is_writable` method to ensure it's properly detecting any unwanted changes in the offer before publishing. It also fixes a bug on using the old `cloudpub` signature for `get_product_by_name`. Depends on: release-engineering/cloudpub#152 Refers to SPSTRAT-630 Signed-off-by: Jonathan Gangi <[email protected]> Assisted-by: Cursor/Gemini
JAVGan
changed the title
Collaborator
Author
|
Please note that this PR depends on release-engineering/cloudpub#152 |
![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:
- Extract the inline filter_out_technical_resources function into a private method to improve readability and enable isolated testing.
- Encapsulate the submission-to-states extraction logic in a helper to reduce complexity and nesting inside ensure_offer_is_writable.
- Consider replacing the generic RuntimeError with a dedicated exception class for offer-writability conflicts to make error handling more precise.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the inline filter_out_technical_resources function into a private method to improve readability and enable isolated testing.
- Encapsulate the submission-to-states extraction logic in a helper to reduce complexity and nesting inside ensure_offer_is_writable.
- Consider replacing the generic RuntimeError with a dedicated exception class for offer-writability conflicts to make error handling more precise.
## Individual Comments
### Comment 1
<location> `src/pubtools/_marketplacesvm/cloud_providers/ms_azure.py:370-373` </location>
<code_context>
+ # The offer might be either on "draft" from initial state or published with new changes.
+ # We just want to prevent unwanted changes from a published offer, so we should not
+ # fail on an offer with "draft" from initial state.
+ if states and states != ["draft"]:
+
+ # Now we should get either the "preview" or "live" content in order to compare with
</code_context>
<issue_to_address>
**suggestion:** The conditional may not handle all possible state combinations robustly.
The current logic may fail if 'states' contains multiple values or if their order varies. Please ensure all valid state scenarios are handled explicitly to prevent unintended outcomes.
```suggestion
# The offer might be either on "draft" from initial state or published with new changes.
# We just want to prevent unwanted changes from a published offer, so we should not
# fail on an offer with "draft" from initial state.
# Only proceed if there are states other than "draft"
if states and not (set(states) == {"draft"}):
```
</issue_to_address>
### Comment 2
<location> `src/pubtools/_marketplacesvm/cloud_providers/ms_azure.py:375-378` </location>
<code_context>
+
+ # Now we should get either the "preview" or "live" content in order to compare with
+ # the draft
+ last_state = "preview" if "preview" in states else "live"
+ offer_latest = self.publish_svc.get_product(product_id, last_state)
+
</code_context>
<issue_to_address>
**suggestion:** Defaulting to 'live' if 'preview' is absent may not cover all cases.
If neither 'preview' nor 'live' exists in `states`, 'live' may be used incorrectly. Add a check or error handling for this scenario.
```suggestion
# Now we should get either the "preview" or "live" content in order to compare with
# the draft
if "preview" in states:
last_state = "preview"
elif "live" in states:
last_state = "live"
else:
raise ValueError(
f"No 'preview' or 'live' state found in states: {states} for product {product_id}"
)
offer_latest = self.publish_svc.get_product(product_id, last_state)
```
</issue_to_address>
### Comment 3
<location> `tests/cloud_providers/test_provider_azure.py:528` </location>
<code_context>
+ diff = diff_offers(fake_product_draft, fake_product_latest)
+ fake_azure_provider.publish_svc.diff_two_offers.return_value = diff
+
+ # The offer_name comes from destination.split("/")[0], which is "product-name"
+ # from azure_push_item.dest[0]
+ expected_err_prefix = "Can't update the offer product-name as it's already being changed: "
</code_context>
<issue_to_address>
**suggestion (testing):** The test does not cover the scenario where the submissions list contains only a draft state.
Please add a test case with only a 'draft' submission to confirm publishing is allowed and no error occurs.
</issue_to_address>
### Comment 4
<location> `src/pubtools/_marketplacesvm/cloud_providers/ms_azure.py:355` </location>
<code_context>
def ensure_offer_is_writable(self, destination: str, nochannel: bool) -> None:
"""
Ensure the offer can be modified and published by this tool.
If the offer's initial state is "draft" it means someone made manual changes in the webui
and we cannot proceed. However, this is just true if the offer hasn't being changed by this
tool, thus we use the Borg to inform us whether we're safe to proceed or not.
Since during the `publish` phase we need to call it two time (one with keep_draft as True to
associate the images to possible multiple plans of the same offer, the other to submit) we
will have a "draft" state that is caused by the tooling, hence the Borg to keep track of
what it touched to disconsider this "draft" as a signal of manual changes.
"""
def filter_out_technical_resources(resources):
"""Remove (Core) VM technical resources from list."""
return [
r
for r in resources
if not (isinstance(r, VMIPlanTechConfig) or isinstance(r, CoreVMIPlanTechConfig))
]
# Gather initial data
offer_name = destination.split("/")[0]
product_id = self.publish_svc.get_productid(offer_name)
submissions = self.publish_svc.get_submissions(product_id)
# Get all submission targets available for the exising offer
states = []
for sub in submissions:
target = sub.target
if target.targetType == "draft" or sub.result == "succeeded":
states.append(target.targetType)
# The offer might be either on "draft" from initial state or published with new changes.
# We just want to prevent unwanted changes from a published offer, so we should not
# fail on an offer with "draft" from initial state.
if states and states != ["draft"]:
# Now we should get either the "preview" or "live" content in order to compare with
# the draft
last_state = "preview" if "preview" in states else "live"
offer_latest = self.publish_svc.get_product(product_id, last_state)
# as well as retrieve the draft content
offer_draft = self.publish_svc.get_product(product_id, "draft")
# We don't want to compute any diff on "technical-configuration"
offer_latest.resources = filter_out_technical_resources(offer_latest.resources)
offer_draft.resources = filter_out_technical_resources(offer_draft.resources)
# Diff the resources to see if there are any unpublished changes
if diff := self.publish_svc.diff_two_offers(offer_latest, offer_draft):
error_msg = (
f"Can't update the offer {offer_name} as it's already being changed: "
f"{diff.pretty()}"
)
raise RuntimeError(error_msg)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge isinstance calls ([`merge-isinstance`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/merge-isinstance/))
```suggestion
if not (isinstance(r, (VMIPlanTechConfig, CoreVMIPlanTechConfig)))
```
</issue_to_address>
### Comment 5
<location> `tests/cloud_providers/test_provider_azure.py:533` </location>
<code_context>
@pytest.mark.parametrize("latest_state", ["preview", "live"])
@patch("pubtools._marketplacesvm.cloud_providers.ms_azure.AzurePublishMetadata")
def test_publish_fails_on_draft_state(
mock_metadata: MagicMock,
latest_state: str,
azure_push_item: VHDPushItem,
fake_azure_provider: AzureProvider,
) -> None:
def diff_offers(offer_left, offer_right):
return DeepDiff(offer_left, offer_right)
fake_product_draft = Product.from_json(
{
"$schema": "https://product-ingestion.azureedge.net/schema/resource-tree/2022-03-01-preview2", # noqa: E501
"root": "product/product/ffffffff-ffff-ffff-ffff-ffffffffffff",
"target": {"targetType": "draft"},
"resources": [
{
"$schema": "https://schema.mp.microsoft.com/schema/product/2022-03-01-preview3",
"id": "product/ffffffff-ffff-ffff-ffff-ffffffffffff",
"identity": {"externalId": "fake-product"},
"type": "azureVirtualMachine",
"alias": "Fake Product",
},
],
}
)
fake_product_latest = Product.from_json(
{
"$schema": "https://product-ingestion.azureedge.net/schema/resource-tree/2022-03-01-preview2", # noqa: E501
"root": "product/product/ffffffff-ffff-ffff-ffff-ffffffffffff",
"target": {"targetType": latest_state},
"resources": [],
}
)
# Create submissions based on latest_state parameter
# For "preview" case, include a preview submission; for "live" case, include a live submission
base_submissions = [
{
"$schema": "https://schema.mp.microsoft.com/schema/submission/2022-03-01-preview2",
"id": "submission/a3230c00-ba71-11f0-b2c0-6e5361048fa9/0",
"product": "product/product/ffffffff-ffff-ffff-ffff-ffffffffffff",
"target": {"targetType": "draft"},
"lifecycleState": "generallyAvailable",
},
]
# Add submission for the latest_state (preview or live)
if latest_state == "preview":
base_submissions.append(
{
"$schema": "https://schema.mp.microsoft.com/schema/submission/2022-03-01-preview2",
"id": "submission/a3230c00-ba71-11f0-b2c0-6e5361048fa9/1152921505700026743",
"product": "product/product/ffffffff-ffff-ffff-ffff-ffffffffffff",
"target": {"targetType": "preview"},
"lifecycleState": "generallyAvailable",
"status": "completed",
"result": "succeeded",
"created": "2025-10-23T03:00:00.0000000Z",
}
)
else: # live
base_submissions.append(
{
"$schema": "https://schema.mp.microsoft.com/schema/submission/2022-03-01-preview2",
"id": "submission/a3230c00-ba71-11f0-b2c0-6e5361048fa9/1152921505700026744",
"product": "product/product/ffffffff-ffff-ffff-ffff-ffffffffffff",
"target": {"targetType": "live"},
"lifecycleState": "generallyAvailable",
"status": "completed",
"result": "succeeded",
"created": "2025-10-23T03:00:00.0000000Z",
}
)
fake_submissions = [ProductSubmission.from_json(x) for x in base_submissions]
# Ensure allow_draft_push is False so ensure_offer_is_writable is called
fake_azure_provider.allow_draft_push = False
fake_azure_provider.publish_svc.get_productid.return_value = "fake-product"
fake_azure_provider.publish_svc.get_submissions.return_value = fake_submissions
fake_azure_provider.publish_svc.get_product.side_effect = [
fake_product_latest,
fake_product_draft,
]
diff = diff_offers(fake_product_draft, fake_product_latest)
fake_azure_provider.publish_svc.diff_two_offers.return_value = diff
# The offer_name comes from destination.split("/")[0], which is "product-name"
# from azure_push_item.dest[0]
expected_err_prefix = "Can't update the offer product-name as it's already being changed: "
# Use re.escape only for the prefix, then match the diff output flexibly
# The diff.pretty() output may contain brackets and other special regex characters
expected_err = re.escape(expected_err_prefix) + ".*"
with pytest.raises(RuntimeError, match=expected_err):
fake_azure_provider.publish(azure_push_item, nochannel=False, overwrite=False)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use f-string instead of string concatenation ([`use-fstring-for-concatenation`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-concatenation/))
```suggestion
expected_err = f"{re.escape(expected_err_prefix)}.*"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| diff = diff_offers(fake_product_draft, fake_product_latest) | ||
| fake_azure_provider.publish_svc.diff_two_offers.return_value = diff | ||
|
|
||
| # The offer_name comes from destination.split("/")[0], which is "product-name" |
There was a problem hiding this comment.
suggestion (testing): The test does not cover the scenario where the submissions list contains only a draft state.
Please add a test case with only a 'draft' submission to confirm publishing is allowed and no error occurs.
| return [ | ||
| r | ||
| for r in resources | ||
| if not (isinstance(r, VMIPlanTechConfig) or isinstance(r, CoreVMIPlanTechConfig)) |
There was a problem hiding this comment.
suggestion (code-quality): Merge isinstance calls (merge-isinstance)
Suggested change
| if not (isinstance(r, VMIPlanTechConfig) or isinstance(r, CoreVMIPlanTechConfig)) | |
| if not (isinstance(r, (VMIPlanTechConfig, CoreVMIPlanTechConfig))) |
| expected_err_prefix = "Can't update the offer product-name as it's already being changed: " | ||
| # Use re.escape only for the prefix, then match the diff output flexibly | ||
| # The diff.pretty() output may contain brackets and other special regex characters | ||
| expected_err = re.escape(expected_err_prefix) + ".*" |
There was a problem hiding this comment.
suggestion (code-quality): Use f-string instead of string concatenation (use-fstring-for-concatenation)
Suggested change
| expected_err = re.escape(expected_err_prefix) + ".*" | |
| expected_err = f"{re.escape(expected_err_prefix)}.*" |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 commit reworks the
AzureProvider.ensure_offer_is_writablemethod to ensure it's properly detecting any unwanted changes in the offer before publishing.It also fixes a bug on using the old
cloudpubsignature forget_product_by_name.Depends on: release-engineering/cloudpub#152
Refers to SPSTRAT-630
Summary by Sourcery
Rework AzureProvider’s draft detection to leverage submission data and offer diffs rather than shared state, remove the Borg singleton, update cloudpub API usage, and adjust tests accordingly
Bug Fixes:
Enhancements:
Tests: