Add support for starting the application through CMD (instead of ENTRYPOINT). #33037
Conversation
…YPOINT). The new ContainerAppCommand and ContainerAppCommandArgs properties are used to hold the command line to start the application. The ContainerAppCommandInstruction controls how the command line is emitted. The default behavior is to emit an ENTRYPOINT when the base image has no ENTRYPOINT, and a CMD when the base image has an ENTRYPOINT (to use the base image ENTRYPOINT). This also adds a ContainerCmd property, which can be used to directly set the image CMD.
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
@baronfel this is a WIP, it outlines most of the implementation. I want to share what I have so we can do an early feedback round. ptal. |
| /// <summary> | ||
| /// Looks up a localized string similar to CONTAINER2006: Unknown AppCommandInstruction '{0}'. Valid instructions are {1}.. | ||
| /// </summary> | ||
| internal static string UnknownAppCommandInstruction { |
There was a problem hiding this comment.
I've added this manually again.
Maybe it only works on Windows?
@baronfel when you run the top-level build script on Windows, does it update this file with new strings?
|
Sorry, finally getting to this. Will take a look this afternoon and give feedback. |
tmds
changed the title
Take your time. I got just got it in a state that is good for a review. I'll extend the unit test with some more cases, I don't have any other changes in mind currently. |
| if (baseImageEntrypoint.Length > 0) | ||
| { | ||
| Log.LogWarningWithCodeFromResources(nameof(Strings.BaseEntrypointOverwritten)); | ||
| } |
There was a problem hiding this comment.
Rather than change between Entrypoint and Cmd based on the presence of a base entrypoint, for deterministic behavior, I've opted to prefer Entrypoint and print a warning if the base image defines an entrypoint already (as is the case with Red Hat images).
| /// <summary> | ||
| /// Default arguments passed. These can be overridden by the user when the container is created. | ||
| /// </summary> | ||
| public ITaskItem[] DefaultArgs { get; set; } |
There was a problem hiding this comment.
This roughly corresponds to what dotnet/sdk-container-builds#11 called ContainerCommand.
baronfel
added
the
Area-Containers
@baronfel I've just extended the test. If you look at it, it should describe well how these different properties combine together to determine the application image Entrypoint and Cmd. |
|
Thanks @tmds - this LGTM overall, but I'm going to wait to merge until Friday. We're going over a batch of the open Containers PRs for detailed review then and I want to make sure the rest of the team is on board with the changes. |
| @@ -240,4 +242,108 @@ public void Dispose() | |||
| { | |||
| _cancellationTokenSource.Dispose(); | |||
| } | |||
|
|
|||
| internal (string[] entrypoint, string[] cmd) DetermineEntrypointAndCmd(string[]? baseImageEntrypoint) | |||
There was a problem hiding this comment.
we need to replicate this logic in the net472 version of the command - the validations there are only looking at EntryPoint/EntrypointArgs and so are breaking after this PR.
There was a problem hiding this comment.
Where is the net472 version implemented?
There was a problem hiding this comment.
It's the sibling CreateNewImageToolTask.cs file next to CreateNewImage.cs - this task invokes the containerize exe with CLI variables, but does validation of those variables in its Execute method so that users get good MSBuild diagnostics.
The ToolTask and the 'normal' task share the same Interface.fs partial class, but are conditionally compiled for net472 or net8.0, respectively.
There was a problem hiding this comment.
Got it. Shall I look into making the necessary changes?
There was a problem hiding this comment.
That would be awesome! I think it would be a matter of
- pushing this entrypoint-detection method down into the .Interface.cs class
- Using it in both CreateNewImage tasks separately
- Emitting the new CLI args for the entrypoint args/command/etc in the ToolTask's 'GenerateCommandLine' method
- A brief look at emitting nice diagnostics from the ToolTask
There was a problem hiding this comment.
I'll look into it. Thanks for the outline.
The new ContainerAppCommand and ContainerAppCommandArgs properties are used to hold the command line to start the application.
The ContainerAppCommandInstruction controls how the command line is emitted.
The default behavior is to emit an ENTRYPOINT when the base image has no ENTRYPOINT, and a CMD when the base image has an ENTRYPOINT (to use the base image ENTRYPOINT).
This also adds a ContainerCmd property, which can be used to directly set the image CMD.
Fixes dotnet/sdk-container-builds#398.