azd infra synth Improvements

[ellismg]

Opening as a draft of my WIP branch while I finish this up - there are some questions that I have.

---

synth: Take infra section of azure.yaml into account

azd infra synth was not taking the path to the shared infrastructure
folder inor the name of the root module from the infra of
azure.yaml into account, and instead just always generated files
into the infra folder at the root of the project.

Since these were getting generated to the wrong place, they were
ignored during future azd provision runs.

We now take these values into account when generating filenames and
paths.

synth: Warn when AppHost has changed since generation

This changes adds a commented to files generated by azd infra synth
to include a hash of the manifest that was used to generate the
infrastructure. When we use these files later, we compare the hash
recorded in the file to the hash of the current manifest and issue a
warning.

[ellismg]

Still some work to be done here - we need to plumb the warning into the provision side of the house as well.

I'm not sure what the output should look like exactly and the copy is also pretty bad.

Here's the output of azd deploy with this change, where I ran azd infra synth:

I'm sure there's something better we can be saying here. I'm not sure if we want to tell the user to re-run azd infra synth since that would just cause them to discard any changes they had made "by hand".

One frustrating thing to me is that the warning is written at the end of the operation (and only upon success) but really I would like to issue the warning as soon as I notice it (after reading the yaml template) but we don't have a great way to do that today with the console abstraction and the task abstraction.

Something else that is unfortunate right now is that the warning is overly pessimistic. For example, in the above I had taken the aspire starter project:

var builder = DistributedApplication.CreateBuilder(args);

var apiService = builder.AddProject<Projects.AspireStarterChanges_ApiService>("apiservice");

builder.AddProject<Projects.AspireStarterChanges_Web>("webfrontend")
    .WithExternalHttpEndpoints()
    .WithReference(apiService);

builder.Build().Run();

and then just added a .WithEnvironment("FOO", "BAR")

to the builder.AddProject<Projects.AspireStarterChanges_Web>("webfrontend") chain.

Ideally, we would have not issued a warning for the apiservice project, since there was no change to the parts of the manifest it cares about, but right now manifest hash generation is very simple - we just take the entire document, remove whitespace and sha256 it.

We also aren't taking the contents of any bicep files referenced by the manifest into account - but really we should (normalizing newlines?)

But - opening this up for discussion.

@davidfowl Did you have thoughts about what you wanted the UX to feel like here?

[ellismg]

but right now manifest hash generation is very simple - we just take the entire document, remove whitespace and sha256 it.

The fact that the end to end integration tests failed on each type of OS and each of them computed a different hash makes me think we will need to do something more clever.

[davidfowl]

@davidfowl Did you have thoughts about what you wanted the UX to feel like here?

I was thinking that it would prompt before doing anything much like azd infra synth prompts today before overwriting files. The prompt would offer 2 choices, "Continue without new changes" and "Re-run infra synth and deploy"

[ellismg]

The prompt would offer 2 choices, "Continue without new changes" and "Re-run infra synth and deploy"

The thing I am struggling with, @davidfowl, is that I'm not sure that "Re-run infra synth and deploy" is something reasonable to pick? If someone did this, it would remove all the customizations they had applied by hand after infra synth, like an "accept theirs" in a merge conflict. Would we expect you'd just want to deploy that without making any changes or even looking at what got generated?

The idea of the prompt during deployment is interesting, it should just work, but we will need to also look at how this feels in VS when using their Aspire Deployment flow. When we were just printing messages it made sense to me how this would work in VS, but if we throw up a prompt, I am less certain about how it will work. We do have prior art here for prompting during the up flow but we should walk through the scenario and validate it both works and the UX is reasonable.

In non-interactive scenarios, like CI, I am guessing we would just "Continue without new changes" and log the warning. Feels better than aborting the whole operation.

[davidfowl]

The thing I am struggling with, @davidfowl, is that I'm not sure that "Re-run infra synth and deploy" is something reasonable to pick? If someone did this, it would remove all the customizations they had applied by hand after infra synth, like an "accept theirs" in a merge conflict. Would we expect you'd just want to deploy that without making any changes or even looking at what got generated?

I see, I think you're right. I think it should fail then instead of prompt. It's very likely a mistake and you should be forced to take an explicit action like specify a --overwrite flag, instead of showing a warning and deploying anyways.