-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
.NET Monitor: Use tarballs instead of tool package #4356
Conversation
@@ -1 +1 @@ | |||
ENTRYPOINT [ "dotnet-monitor", "collect", "--urls", "https://+:52323", "--metricUrls", "http://+:52325" ] | |||
ENTRYPOINT [ "dotnet", "./dotnet-monitor.dll", "collect", "--urls", "https://+:52323", "--metricUrls", "http://+:52325" ] |
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.
This is a breaking change of the entrypoint of the Docker images. Consider using the apphost (dotnet-monitor
) again; this would require adding the apphost to the tarball which would marginally increase its size.
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.
Why is this considered a breaking change? What has this contract?
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.
Customers who are already using the 6 and 7 images may be overriding the ENTRYPOINT or the CMD instructions. For example, in our Kubernetes sample, the CMD instruction is overridden with args: [ "--no-auth" ]
(in the 6 version) or args: [ "collect", "--no-auth" ]
(in the 7+ version).
Similarly, the ENTRYPOINT may be overridden. If we change what the default entrypoint is, then customers who are overriding it (as is frequently the case for the 6 version since the executable and arguments are all part of the entrypoint) have to adjust to that change as well; in this case, they would have to change the dotnet-monitor
portion of their entrypoint override to dotnet ./dotnet-monitor.dll
.
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.
I always get these different concepts mixed up. I've often clobbered the entrypoint, but not just the args. I didn't even realize that this experience was a thing. That's why I asked. Thanks!
I will wait until all of the inserted builds have tarball generation fully supported before making this change. Until then, I'll close out this PR. |
FWIW ... I reported your tarball use case to the SDK team. This is quite obviously an example of the .NET tools not delivering on a reasonable set of scenarios. |
I think that dotnet/sdk#28923 is akin to what we are trying to achieve with the tarballs but in .NET Tool form. We'd probably stick with tarballs in the Docker images but would likely adopt any RID specific solution that the SDK provides for our .NET tool package. |
Use RID-specific, TFM-specific tarballs instead of .NET tool package to avoid always having to use an installer stage (still necessary for distroless images) and to avoid needing to surgically delete unused files from the .NET tool installation. This change brings .NET Monitor more inline with how the other .NET products are offered and provides a simpler acquisition experience for production scenarios (which typically do not have the .NET SDK available).