Move PublishAsDockerFile to use container resources.#7072
Conversation
This allows full customization of the docker builing process and removes a dependency on the limited docker.v0 resource type. - Updated `DockerfileBuildAnnotation` to allow nullable build arguments. - Modified `WithBuildArg` to accept nullable values. - Updated API documentation in `PublicAPI.Shipped.txt` and `PublicAPI.Unshipped.txt`. - Revised tests in `ManifestGenerationTests.cs` and `PublishAsDockerfileTests.cs` to align with new behavior. - Adjusted `SchemaTests.cs` to use temporary context paths for Dockerfile testing. Fixes #4067
davidfowl
requested review from
JamesNK,
eerhardt and
sebastienros
and removed request for
mitchdenny
davidfowl
added
the
area-app-model
- Updated the ExecutableReference's manifest writer even though the resource gets removed from the list. - Updated tests (including the python tests)
| } | ||
| var container = new ExecutableContainerResource(builder.Resource); | ||
| var cb = builder.ApplicationBuilder.AddResource(container); | ||
| cb.WithImage(builder.Resource.Name); |
There was a problem hiding this comment.
For my understanding, why do we need to call WithImage when we call WithDockerfile? Is that just to name the image that will be produced?
There was a problem hiding this comment.
Also, doesn't WithDockerfile just overwrite what we pass here?
There was a problem hiding this comment.
Nah you need aan image name.
There was a problem hiding this comment.
System.InvalidOperationException : The resource 'foo' does not have a container image specified. Use WithImage to specify the container image and tag.
There was a problem hiding this comment.
ah - it's because of the ORDER of the calls above. If you change WithDockerfile to:
return builder.WithAnnotation(annotation, ResourceAnnotationMutationBehavior.Replace)
.WithImage(imageName)
.WithImageRegistry(registry: null)
.WithImageTag("latest");it will work. WithImageRegistry is throwing because it can't find the ContainerImageAnnotation.
There was a problem hiding this comment.
I added a comment
There was a problem hiding this comment.
I think we should fix WithDockerfile.
The current proposed code is confusing because you are going to get a different Image name that what is being specified here - since WithDockerfile is going to overwrite it.
There was a problem hiding this comment.
I still think the code you are adding in this PR is confusing. But I'm not going to block on it.
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
2 participants
Description
This allows full customization of the docker building process and removes a dependency on the limited docker.v0 resource type.
DockerfileBuildAnnotationto allow nullable build arguments.WithBuildArgto accept nullable values.ManifestGenerationTests.csandPublishAsDockerfileTests.csto align with new behavior.SchemaTests.csto use temporary context paths for Dockerfile testing.Fixes #4067
Checklist
<remarks />and<code />elements on your triple slash comments?breaking-changetemplate):doc-ideatemplate):