Adds middleware to ensure azd environment is always available #3940
Conversation
wbreza
requested review from
ellismg,
vhvb1989,
hemarina and
weikanglim
as code owners
| ) | ||
|
|
||
| // EnvironmentMiddleware is a middleware that loads the environment when not readily available | ||
| type EnvironmentMiddleware struct { |
There was a problem hiding this comment.
The idea of handling environment with a middleware is somehow confusing to me.
I might be totally wrong, but, in my mind, a middleware is an optional enhancement for the system itself. For example, the debug or telemetry middlewares makes total sense b/c the system can work with or without them enabled. Even hooks as a middleware make sense, because it means you can run azd with hooks disabled, just by controlling the midleware.
However, thinking about environment, the system will not operate properly if this middleware is not enabled. Hence, it seems like, if having an environment is a requirement, it should not be a middleware.
There was a problem hiding this comment.
A middleware does not need to be an optional/additive component. It is a great model to author cross cutting concerns on the top of an application that deals with request/response (RPC) rather than having to litter these concerns across an application code base.
Similar to how we are now handling our commands/actions final UX output that handles success & error responses we can now extend the how environments are initialized in a single clear component.
There was a problem hiding this comment.
handling our commands/actions final UX output
If we remove the middleware for handling UX, the command would still work, right? It will not have the same UX output, but it would complete from start to finish.
That's the way I see middlewares, you can add them to decorate functionality, but the core functionality never depends on having a middleware.
Core functionality is usually controlled by execution-policies or constrains, which are mapped to business-logic.
For some commands, having an existing environment is an execution policy/constrains which makes the app fail if the policy is not satisfied.
There was a problem hiding this comment.
It doesn't matter what you call it - they are all the same. Execution policies, middleware, etc. They all have the opportunity to run within the request pipeline to modify the request/response as it traverses through a command/request, etc.
We should not have the belief that middleware are only optional. An example of this is the hooks middleware - if you don't register the hooks middleware then azd hooks features does not work.
There was a problem hiding this comment.
I wonder if the difference here is that we aren't just setting request/response context properties. What we're doing is actually updating components in the IoC container. This update happens in the "cmd/middleware" system which makes it harder to track down. Perhaps this is inline with @ellismg's comment here about how we can ensure command middleware/resolvers run in a way that doesn't modify the container in an unexpected way.
I'm wondering if we only modified hooks to switch its dependency of *lazy.Lazy[*environment.Environment] to *environment.Environment, would that sufficiently fix the reported issue?
There was a problem hiding this comment.
Since the environment may or may not exist at this point in time we cannot inject a raw *environment.Environment and would need to use *lazy.Lazy[*environment.Environment].
I added some more verbose feedback in response to @ellismg's post.
There was a problem hiding this comment.
My understanding is that the current change in this PR also forces environment creation when hooks run. In my mind, we did two things in this PR:
- Change
*lazy.Lazy[*environment.Environment]to*environment.Environmentinhooks.go(forces environment creation when needed) - Move the initialization logic to be managed by a middleware instead of a resolver
And we could drop 2 but 1 still works the same. Please feel free to correct my understanding if it isn't quite right.
There was a problem hiding this comment.
I wrote a change in #4011 which I tested did address the issue using the same approach in this PR, just without middleware created (avoids number 2 as I described above).
There was a problem hiding this comment.
A few small comments.
I want to make sure I understand how this change is working - my general working theory here is that the middleware is injected into every command and ends up running before the underlying action func.
On any command which might need the environment (i.e. all commands that don't have EnvironmentOptions.Optional set, this middleware is run and ends up doing the common logic of loading an environment, and then sets the value inside the *lazy.Lazy[*environment.Environment] that is injected into the middleware. This value is a singleton, and updating it means that every other component that injected an *lazy.Lazy[*environment.Environment] sees this value. In addition, components which inject just *environment.Environment end up running code that plucks this value from the singleton *lazy.Lazy[*environment.Environment] to get the value, so they also see this updated version.
Is that right? I'm a little concerned about when the resolver functions for actions actually end up running and if there is a case where a resolver function takes an *environment.Environment and it ends up getting injected a value that has been constructed before the first set of middleware that sets everything up has run.
Do I have the right general idea of how this feature works? If so, are there guards in the system that prevent the thing I'm concerned about?
BUT - since middlewares run before actions the middlewares will be the first consumer of the environment. If there is a valid environment already returned by the IoC container then this middleware is a noop and there isn't any mutation of the environment happening. This middleware really only comes into play when a new environment is being created interactively from the use of the
Yes, we just need to all be aware of the order of operations.
|
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
microsoft-github-policy-service
bot
added
the
no-recent-activity
|
Hi @wbreza. Thank you for your interest in helping to improve the Azure Developer CLI 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. |
Adds an
EnvironmentMiddlewarecomponent that will validate and create azd environment as needed.Resolves #3920