First cut at refactoring DefaultClientOptionsBuilder #3270
Conversation
|
Part of me is starting to wonder if instead of injecting the pipeline, the credential, and a type to construct the options bag, we really should be injecting the client type from the SDK itself. That feels like it would simplify a bunch of these components and we'd still be able to mock reasonably. |
@ellismg -- We intentionally inject a configuration object because there are some scenarios where the options differ from a "stock" client that would be injected from the container. The biggest differentiator in this case is the user agent. If we want the same user agent then injecting the client directly would be more reasonable. An example of a hardcoded user agent: https://github.com/Azure/azure-dev/pull/3270/files#diff-3e7d63cbbc6072a823c88293f3fc3762d0167fb5a833209412eaff0dfde90161R174 Also, |
| func NewContainerAppService( | ||
| credentialProvider account.SubscriptionCredentialProvider, | ||
| httpClient httputil.HttpClient, | ||
| clock clock.Clock, | ||
| defaultClientOptionsBuilder *azsdk.ClientOptionsBuilderFactory, | ||
| ) ContainerAppService { | ||
| return &containerAppService{ | ||
| credentialProvider: credentialProvider, | ||
| httpClient: httpClient, | ||
| userAgent: azdinternal.UserAgent(), | ||
| clock: clock, | ||
| credentialProvider: credentialProvider, | ||
| clock: clock, | ||
| defaultClientOptionsBuilder: defaultClientOptionsBuilder, |
There was a problem hiding this comment.
Could our wrapping service expect the fully hydrated ContainerAppsClient here instead of the options used to build the client?
Then we can move the instantiation of these clients to the IoC container and simplify the external dependencies in our services?
Or maybe not since many of these constructors take potentially transient data like subscriptionId parameters.
There was a problem hiding this comment.
My biggest concern is these call-time variables like subscriptionId in this case and, in almost all other cases, ctx. If we can accept these from the container then we can, as you point out, inject fully hydrated clients.
In the case of service_manager.go's Deploy func, the subscriptionId sent down to container_app.go functions comes from an injected environment.Environment object.
In these situations it may be possible to treat subscriptionId as an injectable type which extends string (similar to httputil.UserAgent)
But to make such a change we'd need to be very confident that the work done to arrive at the configurations in the container are accurate for use later on.
Also, we have to assume that ctx is never modified in places where it's injected and ignore a ctx which could be provided by a caller.
There was a problem hiding this comment.
Met w/ @chlowell yesterday. A few takeaways:
ctxshould not be bound to policy (we're not binding thectxobject directly to the policy, rather we're using it to transport the trace id)... instead we should use thectxat call time (instead of creation time as we're doing today). This amounts to changing the policy to use the context available in theDofunc.- The design of binding
subscriptionIdto an Azure SDK client at creation is in accordance with the accepted patterns. (@wbreza and I also discussed that we think thesubscriptionIdis unlikely to change during a single execution ofazd)
To that end I'm going to:
- Refactor
correlation_policy.goto use the ctx from the request at call-time - Create a type for
subscriptionIdwhich can be populated using the IoC container (and other types as needed)
This should enable the injection of hydrated clients. Updates coming soon.
cli/azd/cmd/container.go
Outdated
| // TODO: File this registration in a reasonable place | ||
| container.MustRegisterSingleton(func( | ||
| ctx context.Context, | ||
| env *environment.Environment, | ||
| ) environment.SubscriptionId { | ||
| return environment.SubscriptionId(env.GetSubscriptionId()) | ||
| }) |
There was a problem hiding this comment.
I don't think we're adding enough value by introducing the SubscriptionId type alias here. We're only ever pulling from the azd env here, so you might as well just inject env into the client resolver functions below.
@ellismg -- It took a while but I was able to refactor a lot of client injections (the bulk of which are in
A couple of fixes here might look like:
|
danieljurek
force-pushed
the
djurek/default-client-options-builder
branch
from
8e0d998
to
d187bb3
Compare
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSIContainerDocumentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
|
This is done finally in #3433 |
This includes an intentionally terrible name. Not blocking on coming up with a name at this moment.
This would be an antecedent to #2897