Skip to content

sdk - rework LRO and ProvisioningState pollers to match ARM spec for terminal statuses #1221

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

Merged
merged 3 commits into from
Jul 16, 2025

Conversation

jackofallops
Copy link
Member

@jackofallops jackofallops commented Jul 15, 2025

This PR reworks how the SDK deals with Long Running Operation polling by removing custom "Terminal" statuses from consideration. ARM allows arbitrary values in the Status fields, including custom values supplied by users. Since it is not possible to know all possible values in advance, this PR delegates the success/failure of an operation to the service returning one of the ARM specified Terminal Statuses in the LRO response. Any other values are considered "InProgress" for the purposes of Operation Status.

This PR includes the changes in #1124 and supersedes it.

@jackofallops jackofallops requested a review from a team as a code owner July 15, 2025 09:57
@github-actions github-actions bot added the release-once-merged The SDK should be released once this PR is merged label Jul 15, 2025
Copy link
Contributor

@sreallymatt sreallymatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ✅

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just have one concern that I've left a comment on but if that isn't an issue, this looks good!

status = string(result.Status)
}
if string(result.Properties.ProvisioningState) != "" {
// if the result has a provisioningState field, we should prioritise that
if statusIsTerminal(result.Properties.ProvisioningState) {
status = string(result.Properties.ProvisioningState)
}
if status == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the status is something like one of the removed checks Updating, it wouldn't pass the statusIsTerminal check and we'd hit this check and just say that Polling Succeeded when it might still be updating.

Am I reading this right and is that the route we want to go?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, the spec (I'll try and find the link) states that the ProvisioningState takes priority on provisioning operations. This behaviour isn't actually changing here, we've always prioritised this over result.Status, the mechanism that checks the value has just been updated to accommodate the shift to only acting on states that are actually terminal in terms of the operation.

@jackofallops jackofallops merged commit d60f910 into main Jul 16, 2025
6 checks passed
@jackofallops jackofallops deleted the f/lro-rework-for-terminal-status-spec branch July 16, 2025 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-once-merged The SDK should be released once this PR is merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants