POC: azd workflow run command #2927
Conversation
|
Will hooks still be triggered? Why do we need "command" in each item? What commands are supported? Only built in ones? |
Yes, hooks will still be triggered for any
My plan was to keep this open to support other types of workflow elements. Maybe additional custom scripts that you want to orchestrate outside of hooks
Any |
|
Thinking about UX. How would a user discover all the workflows they can override and then all the commands they can re-arrange? |
I'm not sure we need anything in the CLI to discover the workflows/order. Hopefully docs will be enough for this but also happy to be wrong here. If we did I could see us having commands like |
Maybe we make it easy for folks to see what commands azd up executes by default? Maybe that is in help? |
Yes, we do provide this in help but 
|
Can you do a PR for it? |
| args := strings.Split(step.Command, " ") | ||
| args = append(args, step.Args...) | ||
|
|
||
| rootCmd.SetArgs(args) |
There was a problem hiding this comment.
This is very interesting. I think the decision to use Cobra here instead of the actions directly probably has some deep implications. Part of me wonders if this strategy would allow us over time to move towards something more first class is Cobra. At some level the action interface stuff has always felt like ceremony to me. We needed it before because we wanted to have up be a sequence of actions. It never really panned out in a way I was happy with.
I wonder if this insight could help us there.
An interesting thing to consider is how this might behave differently from the previous implementation. I'm guessing Pre and Post run stuff in Cobra would happen more often now? Perhaps we are not exploiting that much anymore.
There was a problem hiding this comment.
A few reasons why the cobra commands were chosen to run here.
- Cobra already has support for find the correct command and executing it with the specified args
- This allows us to leverage the existing lifecycle for how are actions are executing including:
- Runs any defined hooks for a command
- Already knows how to look up the
azdaction for command - Works with our telemetry system (via hooks)
I believe the separation of the cobra / action layers still add value here. I think of the cobra layers as the controller layer which then forwards down specific requests to our business tier. There could still be an opportunity to improve the current design/implementation of the action dispatching to be more tightly coupled with the cobra pre/post support instead of our custom implementation in the middleware running.
Yes, any root cobra pre/post scripts would get run more often known in this azd up scenario or any new workflow that was authored by the user. But looking at them this mostly just check the cwd and I wouldn't be worried about it from any performance point of view.
There was a problem hiding this comment.
Do you have a sense for what happens (or what should happen) if you pass something like --environment foo to these sub commands? I sort of have no intuition as to what would happen here. I think the user's intention would be for that workflow to explicitly pass an environment (perhaps up always targets a production environment or something)? I think this is something that the current implementation would allow, because we end up leveraging all of the cobra parsing. I have no idea if this would actually work sensibly, because I have no real intuition about what state ends up getting shared in this scenario and I know that specifically the --environment flag is deeply wed into how our IoC resolver logic works.
Part of me thinks that building on top of cobra here is the right move, but I'm still struggling to think through things like the above (and the fact that cobra argument parsing sort of becomes part of the API of these workflows) and part of me is thinking we should instead be doing something like naming (some of, all of?) the actions we have as "commands" and resolving them from the IoC container and invoking them (and perhaps being finer grained in what arguments can be used here)?
There was a problem hiding this comment.
This POC was intentionally open to let the user specify whatever command/arguments they want. This does leave things open to shoot yourself in the foot.
Should the users be specifying different --environment flags in each command call within a workflow - probably not.
Should we put some guardrails in place around this? - I don't know if it's worth it right now.
I'd rather not limit what command, arguments the user should specify. This leaves things open to support more advanced power user scenarios or having to go back to make adjustments for new commands or variations of what they can run.
There was a problem hiding this comment.
I like the model we present to users here and I think you're right, the simple model of "it behaves as if you had just run the command" makes sense to me.
I do think we need to think about how global flags flow from the "outer" azd into the "inner" azd. For example, if you have two environments, dev and prod and you have dev selected but run azd --environment prod up, I think we want each step in the workflow to behave as if --environment dev was passed. It's not obvious to me that this will happen with the current implementation. And, if for some reason someone added "--environment, prod" to some command in the workflow, that should override the --environment dev from the "outer" azd.
| @@ -55,20 +55,20 @@ import ( | |||
| "golang.org/x/exp/slices" | |||
| ) | |||
|
|
|||
| // Registers a singleton action initializer for the specified action name | |||
| // Registers a transient action initializer for the specified action name | |||
There was a problem hiding this comment.
Should we move everything to transient? I'll admit I struggle to understand the lifetime strategy, why did some of these have to change but others didn't?
Is the use of singletons buying us a to today? Part of me thinks we should be limiting the number of singletons except in cases where they hold state that is expensive to compute or should be shared across multiple components. That feels like very little of azd today (perhaps just like our tool cache abstraction?)
There was a problem hiding this comment.
Since we are introducing a workflow concept this allows users to author their own workflows. This could then allow the user to run the same azd command multiple times. Consider something like
# Custom azd workflow
azd provision
azd package api
azd deploy api
azd package web
azd deploy webWe wouldn't want the same package or deploy commands to share any transient state like args, etc so it is safer to ensure that when the actions are created they always create a new instance.
Having singleton lifetime on well known components does still provider us value and gives us opportunities to reuse components, implement caching when needed. This does however require some additional cognitive load for developers to understand what the lifetime to chose when developing new components.
One such example of the above is if a workflow calls azd package then azd deploy at some point we don't want to re-package during deploy and we'd want to leverage any caching that we can to use previously packaged assets.
| return nil, err | ||
| u.projectConfig.Workflows["up"] = defaultUpWorkflow | ||
| } else { | ||
| u.console.Message(ctx, output.WithWarningFormat("WARNING: Running custom 'up' workflow from azure.yaml")) |
There was a problem hiding this comment.
I do not think we should warn here. If someone needs to change what up means, hitting them with a warning every time they run up for the rest of time is not going to delight them. If you are concerned about people not knowing what is going on, I'd be fine with a log statement so --debug shows what is going on.
There was a problem hiding this comment.
Perhaps warning is not the best choice here to be so upfront in your face. Maybe just a greyed out note? I do feel that some visual indication is helpful to highlight that azd up is not doing its default thing but rather running a custom workflow.
| } | ||
|
|
||
| var defaultUpWorkflow = []*workflow.Step{ | ||
| {Command: "package --all"}, |
There was a problem hiding this comment.
Why is --all not in the Args part of this object?
There was a problem hiding this comment.
I originally created the struct to separate out the command from args but then realized that if we are using cobra to dispatch the command that we don't need to support this separation in the configuration
There was a problem hiding this comment.
I am slightly nervous about us tying ourselves to having to do argument splitting. Maybe we should make Command take a []string and just document it as "the args passed to azd"?
wbreza
force-pushed
the
workflow-poc
branch
2 times, most recently
from
9e0c2dd
to
f0c7bfe
Compare
|
I am excited about how far we were able to push this along, and the pattern with a single From my perspective, I would love to see written treatment from the team, and engineering team discussions around:
All these have broad product UX implications that feels like needs to be written down and carefully selected to avoid feature complexity and the best user experience, in my opinion. |
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSIContainerDocumentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
There was a problem hiding this comment.
I think this is the right direction and I like how we leverage Cobra here. Do have some questions about how the inner and outer instances of azd view the world, but I think this is what we are going to want as a way to allow folks to change what up means.
I do think we'll want a way to run some external program (likely conditionalized on posix/non-posix similar to hooks) in a workflow at some point.
| } | ||
|
|
||
| var rootCmd *cobra.Command | ||
| if err := a.serviceLocator.ResolveNamed("root-cmd", &rootCmd); err != nil { |
There was a problem hiding this comment.
Just wondering - do we need to use the service locator, or could we have just injected the cobra.Command somehow?
| for _, step := range workflow { | ||
| childCtx := middleware.WithChildAction(ctx) | ||
|
|
||
| args := strings.Split(step.Command, " ") |
There was a problem hiding this comment.
Slightly nervous about the splitting here, just from a space escaping point of view. Part of me thinks we should just have Command's value be the "[]string" that we pass to .SetArgs. If we want to have better ergonomics, I would be fine to say the type is either string | []string (I think we can handle that in the marshaling code) and when it is string we just do the splitting based on" "?
| args := strings.Split(step.Command, " ") | ||
| args = append(args, step.Args...) | ||
|
|
||
| rootCmd.SetArgs(args) |
There was a problem hiding this comment.
I like the model we present to users here and I think you're right, the simple model of "it behaves as if you had just run the command" makes sense to me.
I do think we need to think about how global flags flow from the "outer" azd into the "inner" azd. For example, if you have two environments, dev and prod and you have dev selected but run azd --environment prod up, I think we want each step in the workflow to behave as if --environment dev was passed. It's not obvious to me that this will happen with the current implementation. And, if for some reason someone added "--environment, prod" to some command in the workflow, that should override the --environment dev from the "outer" azd.
| @@ -282,6 +283,8 @@ func Test_CLI_Telemetry_NestedCommands(t *testing.T) { | |||
| continue | |||
| } | |||
|
|
|||
| fmt.Printf("FOO: " + span.Name + "\n") | |||
There was a problem hiding this comment.
Guessing this was debugging stuff you'll remove?
| @@ -299,7 +302,7 @@ func Test_CLI_Telemetry_NestedCommands(t *testing.T) { | |||
| require.Contains(t, m, fields.CmdEntry) | |||
| require.Equal(t, "cmd.up", m[fields.CmdEntry]) | |||
|
|
|||
| require.NotContains(t, m, fields.CmdFlags) | |||
| //require.NotContains(t, m, fields.CmdFlags) | |||
There was a problem hiding this comment.
Should we be concerned about this? I don't really grok what this was asserting. Any ideas?
| } | ||
|
|
||
| var defaultUpWorkflow = []*workflow.Step{ | ||
| {Command: "package --all"}, |
There was a problem hiding this comment.
I am slightly nervous about us tying ourselves to having to do argument splitting. Maybe we should make Command take a []string and just document it as "the args passed to azd"?
This is a POC to allow users to establish their own workflows and executed within a single
azd workflow runcommand.This introduces a very basic concept of a workflow within the context of
azd.Current State
azdcommands are run.azdhooks are still executed for all referenced commandsazure.yamlConfigurationazd upcommandThe
azd upcommand has been updated to run a workflow calledupif it exists.If the workflow does not exist it will use a default workflow of the following:
Running Workflow
In this example we have created a workflow for our own custom command ordering for
upHere we are able to take advantage of the internal caching for package components and the apps are not repackaged during the deploy command.
Future State
In a future state a workflow could provide ability to run other commands / scripts outside of azd. (think GitHub actions)