Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions cli/azd/cmd/actions/action_descriptor.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,13 @@ type CommandGroupOptions struct {
RootLevelHelp RootLevelHelpOption
}

// EnvironmentOptions contains options for the environment flag and initialization
type EnvironmentOptions struct {
// Optional should be set to true when an azd environment is optional within an azd command.
// Well known use cases for this are for `azd init` and `azd show`
Optional bool
}

// Defines the type used for annotating a command as part of a group.
type commandGroupAnnotationKey string

Expand Down Expand Up @@ -186,6 +193,8 @@ type ActionDescriptorOptions struct {
HelpOptions ActionHelpOptions
// Defines grouping options for the command
GroupingOptions CommandGroupOptions
// Defines options for the construction of the azd environment
Environment EnvironmentOptions
}

// Completion function used for cobra command flag completion
Expand Down
47 changes: 5 additions & 42 deletions cli/azd/cmd/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -195,50 +195,13 @@ func registerCommonDependencies(container *ioc.NestedContainer) {
// Register an initialized environment based on the specified environment flag, or the default environment.
// Note that referencing an *environment.Environment in a command automatically triggers a UI prompt if the
// environment is uninitialized or a default environment doesn't yet exist.
container.MustRegisterScoped(
func(ctx context.Context,
azdContext *azdcontext.AzdContext,
envManager environment.Manager,
lazyEnv *lazy.Lazy[*environment.Environment],
envFlags internal.EnvFlag,
) (*environment.Environment, error) {
if azdContext == nil {
return nil, azdcontext.ErrNoProject
}

environmentName := envFlags.EnvironmentName
var err error

env, err := envManager.LoadOrInitInteractive(ctx, environmentName)
if err != nil {
return nil, fmt.Errorf("loading environment: %w", err)
}

// Reset lazy env value after loading or creating environment
// This allows any previous lazy instances (such as hooks) to now point to the same instance
lazyEnv.SetValue(env)
container.MustRegisterScoped(func(lazyEnv *lazy.Lazy[*environment.Environment]) (*environment.Environment, error) {
return lazyEnv.GetValue()
})

return env, nil
},
)
container.MustRegisterScoped(func(lazyEnvManager *lazy.Lazy[environment.Manager]) environment.EnvironmentResolver {
container.MustRegisterScoped(func(lazyEnv *lazy.Lazy[*environment.Environment]) environment.EnvironmentResolver {
return func(ctx context.Context) (*environment.Environment, error) {
azdCtx, err := azdcontext.NewAzdContext()
if err != nil {
return nil, err
}
defaultEnv, err := azdCtx.GetDefaultEnvironmentName()
if err != nil {
return nil, err
}

// We need to lazy load the environment manager since it depends on azd context
envManager, err := lazyEnvManager.GetValue()
if err != nil {
return nil, err
}

return envManager.Get(ctx, defaultEnv)
return lazyEnv.GetValue()
}
})

Expand Down
82 changes: 82 additions & 0 deletions cli/azd/cmd/middleware/environment.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package middleware

import (
"context"
"fmt"

"github.com/azure/azure-dev/cli/azd/cmd/actions"
"github.com/azure/azure-dev/cli/azd/internal"
"github.com/azure/azure-dev/cli/azd/pkg/environment"
"github.com/azure/azure-dev/cli/azd/pkg/environment/azdcontext"
"github.com/azure/azure-dev/cli/azd/pkg/lazy"
)

// EnvironmentMiddleware is a middleware that loads the environment when not readily available
type EnvironmentMiddleware struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@weikanglim weikanglim Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Change *lazy.Lazy[*environment.Environment] to *environment.Environment in hooks.go (forces environment creation when needed)
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't have #1 without #2. The environment middleware runs before the hooks middleware ensuring you can access non-lazy environment in hooks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

lazyAzdContext *lazy.Lazy[*azdcontext.AzdContext]
lazyEnvManager *lazy.Lazy[environment.Manager]
lazyEnv *lazy.Lazy[*environment.Environment]
envFlags internal.EnvFlag
}

// NewEnvironmentMiddleware creates a new instance of the EnvironmentMiddleware
func NewEnvironmentMiddleware(
lazyAzdContext *lazy.Lazy[*azdcontext.AzdContext],
lazyEnvManager *lazy.Lazy[environment.Manager],
lazyEnv *lazy.Lazy[*environment.Environment],
envFlags internal.EnvFlag,
) Middleware {
return &EnvironmentMiddleware{
lazyAzdContext: lazyAzdContext,
lazyEnvManager: lazyEnvManager,
lazyEnv: lazyEnv,
envFlags: envFlags,
}
}

// Run runs the EnvironmentMiddleware to load the environment when not readily available
func (m *EnvironmentMiddleware) Run(ctx context.Context, next NextFn) (*actions.ActionResult, error) {
// We already have an environment, skip loading
// This will typically be the case when an environment has been created from a previous command like `azd init`
env, err := m.lazyEnv.GetValue()
if err == nil && env != nil {
return next(ctx)
}

// Needs Azd context before we can have an environment
azdContext, err := m.lazyAzdContext.GetValue()
if err != nil {
// No Azd context errors will by handled downstream
return next(ctx)
}

envManager, err := m.lazyEnvManager.GetValue()
if err != nil {
return nil, fmt.Errorf("loading environment manager: %w", err)
}

// Check env flag (-e, --environment) and environment variable (AZURE_ENV_NAME)
environmentName := m.envFlags.EnvironmentName
if environmentName == "" {
environmentName, err = azdContext.GetDefaultEnvironmentName()
if err != nil {
return nil, err
}
}

// Load or initialize environment interactively from user prompt
env, err = envManager.LoadOrInitInteractive(ctx, environmentName)
if err != nil {
//nolint:lll
return nil, fmt.Errorf(
"failed loading environment. Ensure environment has been set using flag (--environment, -e) or by setting environment variable 'AZURE_ENV_NAME'. %w",
err,
)
}

// Reset lazy env value after loading or creating environment
// This allows any previous lazy instances (such as hooks) to now point to the same instance
m.lazyEnv.SetValue(env)

return next(ctx)
}
151 changes: 151 additions & 0 deletions cli/azd/cmd/middleware/environment_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
package middleware

import (
"context"
"errors"
"testing"

"github.com/azure/azure-dev/cli/azd/cmd/actions"
"github.com/azure/azure-dev/cli/azd/internal"
"github.com/azure/azure-dev/cli/azd/pkg/environment"
"github.com/azure/azure-dev/cli/azd/pkg/environment/azdcontext"
"github.com/azure/azure-dev/cli/azd/pkg/lazy"
"github.com/azure/azure-dev/cli/azd/test/mocks"
"github.com/azure/azure-dev/cli/azd/test/mocks/mockenv"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
)

func Test_Environment_Already_Exists(t *testing.T) {
expectedEnv := environment.NewWithValues("test", map[string]string{
environment.SubscriptionIdEnvVarName: "SUBSCRIPTION_ID",
})

mockContext := mocks.NewMockContext(context.Background())
azdContext := azdcontext.NewAzdContextWithDirectory(t.TempDir())

middleware, lazyEnv := createMiddlewareForTest(azdContext, expectedEnv, internal.EnvFlag{}, &mockenv.MockEnvManager{})
result, err := middleware.Run(*mockContext.Context, nextFn)
require.NoError(t, err)
require.NotNil(t, result)

actualEnv, err := lazyEnv.GetValue()
require.NoError(t, err)
require.NotNil(t, actualEnv)
require.Equal(t, expectedEnv.Name(), actualEnv.Name())
}

func Test_Environment_No_Azd_Context(t *testing.T) {
mockContext := mocks.NewMockContext(context.Background())

middleware, lazyEnv := createMiddlewareForTest(nil, nil, internal.EnvFlag{}, &mockenv.MockEnvManager{})
result, err := middleware.Run(*mockContext.Context, nextFn)
require.NoError(t, err)
require.NotNil(t, result)

actualEnv, err := lazyEnv.GetValue()
require.Error(t, err)
require.Nil(t, actualEnv)
}

func Test_Environment_With_Flag(t *testing.T) {
expectedEnv := environment.NewWithValues("flag-env", map[string]string{})

mockContext := mocks.NewMockContext(context.Background())
azdContext := azdcontext.NewAzdContextWithDirectory(t.TempDir())
envFlag := internal.EnvFlag{EnvironmentName: expectedEnv.Name()}

envManager := &mockenv.MockEnvManager{}
envManager.On("LoadOrInitInteractive", mock.Anything, mock.Anything).Return(expectedEnv, nil)

middleware, lazyEnv := createMiddlewareForTest(azdContext, nil, envFlag, envManager)
result, err := middleware.Run(*mockContext.Context, nextFn)
require.NoError(t, err)
require.NotNil(t, result)

actualEnv, err := lazyEnv.GetValue()
require.NoError(t, err)
require.NotNil(t, actualEnv)
require.Equal(t, expectedEnv.Name(), actualEnv.Name())
}

func Test_Environment_From_Prompt(t *testing.T) {
expectedEnv := environment.NewWithValues("prompt-env", map[string]string{})

mockContext := mocks.NewMockContext(context.Background())
azdContext := azdcontext.NewAzdContextWithDirectory(t.TempDir())

envManager := &mockenv.MockEnvManager{}
envManager.On("LoadOrInitInteractive", mock.Anything, mock.Anything).Return(expectedEnv, nil)

middleware, lazyEnv := createMiddlewareForTest(azdContext, nil, internal.EnvFlag{}, envManager)
result, err := middleware.Run(*mockContext.Context, nextFn)
require.NoError(t, err)
require.NotNil(t, result)

actualEnv, err := lazyEnv.GetValue()
require.NoError(t, err)
require.NotNil(t, actualEnv)
require.Equal(t, expectedEnv.Name(), actualEnv.Name())
}

func Test_Environment_From_Default(t *testing.T) {
expectedEnv := environment.NewWithValues("default-env", map[string]string{})

mockContext := mocks.NewMockContext(context.Background())
azdContext := azdcontext.NewAzdContextWithDirectory(t.TempDir())
err := azdContext.SetProjectState(azdcontext.ProjectState{
DefaultEnvironment: expectedEnv.Name(),
})
require.NoError(t, err)

envManager := &mockenv.MockEnvManager{}
envManager.On("LoadOrInitInteractive", mock.Anything, mock.Anything).Return(expectedEnv, nil)

middleware, lazyEnv := createMiddlewareForTest(azdContext, nil, internal.EnvFlag{}, envManager)
result, err := middleware.Run(*mockContext.Context, nextFn)
require.NoError(t, err)
require.NotNil(t, result)

actualEnv, err := lazyEnv.GetValue()
require.NoError(t, err)
require.NotNil(t, actualEnv)
require.Equal(t, expectedEnv.Name(), actualEnv.Name())
}

func createMiddlewareForTest(
azdContext *azdcontext.AzdContext,
env *environment.Environment,
envFlag internal.EnvFlag,
mockEnvManager *mockenv.MockEnvManager,
) (Middleware, *lazy.Lazy[*environment.Environment]) {
// Setup environment mocks for save & reload
mockEnvManager.On("Save", mock.Anything, mock.Anything).Return(nil)
mockEnvManager.On("Reload", mock.Anything, mock.Anything).Return(nil)

lazyAzdContext := lazy.NewLazy(func() (*azdcontext.AzdContext, error) {
if azdContext == nil {
return nil, azdcontext.ErrNoProject
}

return azdContext, nil
})

lazyEnvManager := lazy.NewLazy(func() (environment.Manager, error) {
return mockEnvManager, nil
})

lazyEnv := lazy.NewLazy(func() (*environment.Environment, error) {
if env == nil {
return nil, errors.New("environemnt not found")
}

return env, nil
})

return NewEnvironmentMiddleware(lazyAzdContext, lazyEnvManager, lazyEnv, envFlag), lazyEnv
}

func nextFn(ctx context.Context) (*actions.ActionResult, error) {
return &actions.ActionResult{}, nil
}
Loading