Remove 404 retry, add custom ctx with timeout #285
Merged
Conversation
This file contains 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
manicminer
requested changes
Jun 11, 2024
There was a problem hiding this comment.
Hi @MarkDordoy, thanks for looking at this. I think it's good to remove the ConsistencyFailureFunc, and as discussed we can implement a workaround in the AzureAD provider rather than in the SDK
manicminer
added a commit
to hashicorp/terraform-provider-azuread
that referenced
this pull request
Jun 11, 2024
…ner/hamilton` - depends on manicminer/hamilton#285
BrendanThompson
pushed a commit
to BrendanThompson/terraform-provider-azuread
that referenced
this pull request
Jun 18, 2024
…ner/hamilton` - depends on manicminer/hamilton#285
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 Removes the Retryon404 added by @amarbut24 and extends the context timeout.
Since having the 404retry in place, it has done the desired effect of retrying, but what appears to happen is we end up with duplicate apps, which then cause a conflict.
My assumption here is the post to MS Graph takes multiple actions on the backend, which can take time to complete (expecially in US gov, texas data center).
Ive been able to reproduce the same issue using the offical MS Graph powershell cmdlets against US Gov Azure tenants so this issue is not specific to hamilton, but more of an azure issue.
My hope with this PR is extending the context timeout to 45 seconds, will give the client longer to wait for the response. If Azure is the one returning the 404, then there is likely nothing we can do, and Microsoft will need to fix.