Skip to content
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

Add completed as teminal state in sdk/azcore #19057

Closed
wants to merge 3 commits into from

Conversation

duncan485
Copy link

@duncan485 duncan485 commented Sep 7, 2022

When working with the sdk/resourcemanager/costmanagement/armcostmanagement v1.0.0 package I encountered a bug when polling the result of NewGenerateDetailedCostReportClient.BeginCreateOperation.

When the report is succesfully generated by this function, it returns a status of 'Completed', however, 'Completed is not recognized as a terminal state in the underlying sdk/azcore package. This leads to an infinite loop in the PollUntilDone function. The bug can be replicated by taking the example of NewGenerateDetailedCostReportClient.BeginCreateOperation from docs

This PR adds 'Completed' as a recognized terminal state.

package main

import (
	"context"
	"log"

	"github.com/Azure/azure-sdk-for-go/sdk/azcore/to"
	"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/costmanagement/armcostmanagement"
)

func main() {
	cred, err := azidentity.NewDefaultAzureCredential(nil)
	if err != nil {
		log.Fatalf("failed to obtain a credential: %v", err)
	}
	ctx := context.Background()
	client, err := armcostmanagement.NewGenerateDetailedCostReportClient(cred, nil)
	if err != nil {
		log.Fatalf("failed to create client: %v", err)
	}
	poller, err := client.BeginCreateOperation(ctx,
		"providers/Microsoft.Billing/billingAccounts/12345",
		armcostmanagement.GenerateDetailedCostReportDefinition{
			BillingPeriod: to.Ptr("202008"),
			Metric:        to.Ptr(armcostmanagement.GenerateDetailedCostReportMetricTypeActualCost),
		},
		nil)
	if err != nil {
		log.Fatalf("failed to finish the request: %v", err)
	}
	res, err := poller.PollUntilDone(ctx, nil) // INFINITE LOOP HERE
	if err != nil {
		log.Fatalf("failed to pull the result: %v", err)
	}
	// TODO: use response item
	_ = res
}
  • [ x] The purpose of this PR is explained in this or a referenced issue.
  • [ x] The PR does not update generated files.
  • [x ] Tests are included and/or updated for code changes.
  • Updates to CHANGELOG.md are included.
  • MIT license headers are included in each file.

@ghost ghost added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Sep 7, 2022
@ghost
Copy link

ghost commented Sep 7, 2022

Thank you for your contribution duncan485! We will review the pull request and get back to you soon.

@ghost
Copy link

ghost commented Sep 7, 2022

CLA assistant check
All CLA requirements met.

@@ -23,6 +23,7 @@ import (

// the well-known set of LRO status/provisioning state values.
const (
StatusCompleted = "Completed"
Copy link
Member

Choose a reason for hiding this comment

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

While I'm not opposed to adding this, I believe this is in violation of the ARM RPC spec. Let me follow up on that.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I was unaware of the spec. Just encountered the bug and tried to solve it this way :) . Let me know if there is a more elegant solution.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @jhendrixMSFT , any update on this?

If there is a better way we can fix the issue please let me know.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the delay, I just pinged the owners again to get their input on this.

Copy link
Member

Choose a reason for hiding this comment

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

The service is in error here, it shouldn't be returned Completed. I'm following up with the service team to get this fixed.

@ghost ghost added the no-recent-activity There has been no recent activity on this issue. label Nov 18, 2022
@ghost
Copy link

ghost commented Nov 18, 2022

Hi @duncan485. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@ghost ghost closed this Nov 25, 2022
@ghost
Copy link

ghost commented Nov 25, 2022

Hi @duncan485. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing "/reopen" if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the "no-recent-activity" label; otherwise, this is likely to be closed again with the next cleanup pass.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-reported Issues that are reported by GitHub users external to the Azure organization. no-recent-activity There has been no recent activity on this issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants