-
Notifications
You must be signed in to change notification settings - Fork 33
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
Replace Autorest with azcore #3536
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. |
13fcc7e
to
236d37a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3536 +/- ##
==========================================
+ Coverage 58.39% 58.97% +0.57%
==========================================
Files 67 68 +1
Lines 8418 8656 +238
==========================================
+ Hits 4916 5105 +189
- Misses 3054 3081 +27
- Partials 448 470 +22 ☔ View full report in Codecov by Sentry. |
236d37a
to
a556ef4
Compare
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.
This looks promising, also the code is quite easy to read! It's also nice how it's almost a drop-in replacement for the old client.
Some comments are below. I'm slightly concerned some of my findings don't result in test failures - it's hard to trust the test suite to catch all errors, it seems.
11ec5ed
to
b2242bf
Compare
e6aee0a
to
ff8d746
Compare
On the rollout strategy, I suggest we should start with the opt-in strategy: use the old auth by default unless the flag is supplied. We can then seek opportunities for deliberate testing of the feature internally and externally with users that benefit from this change (what are those scenarios?) |
aac2fc4
to
2fc0807
Compare
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.
Overall looks great to me. Couple of minor questions inline.
provider/pkg/provider/provider.go
Outdated
// This client will be regnerated with correct environment and authorizer in Configure. | ||
azureClient: azure.NewAzureClient(azureEnv.PublicCloud, nil, azure.BuildUserAgent(PulumiPartnerID)), | ||
// This client will be regenerated with correct environment and authorizer in Configure. | ||
azureClient: azure.NewAzCoreClient(nil, azure.BuildUserAgent(PulumiPartnerID), cloud.AzurePublic, nil), |
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.
Not for this PR, but perhaps azureClient
should be a pointer and should not be created until we've called configure? Is there some reason we need a default client implementation for use before we've configured the provider that you know of?
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.
The only reason I know is that azureClient
would then be a pointer to an interface which is annoying to use: (*k.azureClient).Get
.
dac0e76
to
acdb285
Compare
It might be a hard sell as the benefits are all internal - get off an unsupported dependency, receive security updates and bug fixes, etc. The next step, basing the auth stack on the new and supported azidentity, should close some user-visible bugs: #2432, #2734, maybe #957. |
bbed6bd
to
2bb2d94
Compare
We'll have to keep the dependency for a while regardless of the default value. It there's little immediate user value (upside), we should probably be even more careful not to break anyone (downside).
Sounds like a great motivation to get somebody to try the new auth after those are shipped. |
2bb2d94
to
a9a162e
Compare
6dbb72b
to
c9cb302
Compare
|
||
// Some APIs are explicitly marked `x-ms-long-running-operation` and we should only do the | ||
// poll for the deletion result in that case. | ||
if asyncStyle != "" { |
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.
It's weird that asyncStyle
is used like a boolean flag here; the value doesn't matter. It's the same in the autorest client.
0880438
to
0968499
Compare
This PR introduces a new implementation of
AzureClient
that's based on azcore instead of the deprecated autorest. It's part of [Epic] Replace deprecated REST and auth packages in Azure Native #3576. The initial commit is by @mikhailshilkov, thanks!The PR is rather large. I've attempted to keep logically meaningful individual commits, so reviewing commit by commit might be helpful.
The high-level view of the changes is:
AzureClient
that uses azcore. TheAzureClient
interface remains unchanged.Location
header. That isn't always a qualified URL, as I've observed, so there's new code to make theLocation
qualified.PULUMI_USE_AUTOREST
environment variable to revert to the old client.Integration testing is essentially provided by the existing test suite since all tests now use the new Azure client.
Unit testing is still rudimentary (although already better than the old client's) and will be extended.