-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
provider/azurerm: Upgrading to v10.0.2 of the Azure SDK #14004
Conversation
0d1e460
to
40d9df4
Compare
Is there currently a priority on upgrading to SDK v10 over merging in additional resources? If you need support, just let me know. |
e621801
to
30f5945
Compare
Remote State tests pass:
|
This is the result of the Azure Service Management Provider tests in this branch - where the outstanding failing tests are currently failing in
This is the result of running (one of) the failing tests against the master branch (once updating the SQL Server version to 12.0 as shown here) - but they all produce the same output:
|
@dominik-lekse sorry for the delay in responding to this comment - missed this!
To provide some context here - unfortunately we were unable to upgrade to Generally speaking these SDK upgrades can be a little painful, however we've never had one that's been delayed for this long - and unfortunately the changes required in upgrading to the new version of the SDK have also been larger than we anticipated, requiring changes in both of the Azure Providers (Service Management & Resource Manager) and the Azure Remote State. Unfortunately this has meant that we've been both blocking several open PR's (e.g. App Services) - as well as made things more complicated for new PR's coming in (given we're tied to an old version of the SDK). There's no priority on upgrading the SDK to v10 over merging new resources - however being unable to upgrade to the latest SDK does put us into an unfortunate position, where every new resource which gets added has to be upgraded to use the new version of the SDK - and the longer we delay this PR, the more painful it gets. In general we tend to roll the SDK upgrade out pretty quickly, however this taken longer than we anticipated, so apologies for any delays here. We're now in a position where the SDK upgrade is complete - so I'd expect this to be merged shortly :) Hope that helps? |
|
||
if resp.StatusCode != http.StatusNotFound { | ||
if resp.StatusCode == http.StatusNotFound { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to return nil if the status is StatusNotFound
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into this, internally the Azure SDK is polling for one of the status codes:
azure.WithErrorUnlessStatusCode(http.StatusNoContent, http.StatusOK, http.StatusAccepted)
As such, I've removed this if statement - as we can lean on the SDK to check for the Status Code errors here (which are returned as an error and will be handled below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a minor nit. I'm running tests and will let you know what failures come up from there
Remote State tests still pass:
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Note: this is blocked on Azure/azure-sdk-for-go#593The SDK upgrade is needed so that we can proceed with #13634 / #12001