-
Notifications
You must be signed in to change notification settings - Fork 176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for container.v1 [Aspire] #4008
Conversation
Looks like this might have some pathing issues: I'm using the playground example (withdockerfile) on my PR: |
I resolved that @mitchdenny , but then I had to implement I've added support to resolve expressions in args referencing parameters. No support for resolving other references for now (like a connection string or an output from a bicep.v0 resource). We can iterate on those after the first iteration here. I am now working on the I think this is about 80% of getting us to fully support the playground samples (I am working with that one as well) |
@mitchdenny , this is now ready to test :) I deployed the |
@mitchdenny , can you give it one more change? I've pushed a new commit to fix the issue you encounter (parameter chicken-egg resolution issue, depending on parameters that were not requested yet [back to the future :)]). Fixed it now. |
Sure I'll give this a shot as soon as I am in front of my computer. |
Looks like it is transient. Probably a temporary network issue (although its happened twice to me). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, in that it seems to build and deploy successfully!
We assume runtime dotnet for containers it seems? We should fix that.
|
Guessing the "fix" here is to just always turn that off and if for some reason someone is using a dotnet based container via a non project resource, they just need to Does that seem okay to you, @davidfowl, or do you think we need a way to express "yeah it's a container.v1, but the thing that it runs is dotnet so please keep this on" in the apphost today? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions/comments but the overall approach seems sound (and good to see that it works for Mitch). Great start here, @vhvb1989.
Co-authored-by: Matt Ellis <[email protected]>
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash:
pwsh:
WindowsPowerShell install
MSI install
Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
We're going to need this but we have to model it in the apphost. Similar to 2 here dotnet/aspire#4491 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats on adding the first .v1
resource type, @vhvb1989. LGTM!
fix: #3980
This PR introduces a general new type to support container.v0, dockerfile.v0, project.v0 and container.v1 by having one single abstraction on top of a container img which can be either:
The new abstraction
genBuildContainer
on this PR is only used forcontainer.v1
at this point. The intention is to simplify the code in the future by removing the individual abstractions [genProject, genDockerfile and genContainer] and then simplify the handling of each case as one only abstraction.container.v1
brings new requirements which were not implemented on azd before and are part of this PR:build args
. Currently scoped to referenceparameter.v0
(azd will fail if there is an expression referencing other resource types)build secrets
. Azd resolves any references from the secrets and creates the--secret id=...
arguments for building a container; azd also set the values for the secrets (when using env var secrets) during runtime while invokingdocker build
.