-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Throw error on PublishContainer when EnableSdkContainerSupport is not true #47047
base: main
Are you sure you want to change the base?
Throw error on PublishContainer when EnableSdkContainerSupport is not true #47047
Conversation
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 don't think we can realistically take this change. Here's a screenshot from a sample I made that illustrates the problem:
When you call a target at a solution-level, the solution propagates that Target call to all projects in the solution build configuration being built. Here, that's a web project and a classlib. Since a classlib cannot be containerized, the target is skipped. If this change was taken, any classlib in a solution would break this use case.
@John-Leitch is there an aspirate issue describing the problem you're seeing? If you are expecting certain images that are not being built, there should be other ways to solve your problem that won't regress important user experiences. |
@baronfel, oof, I should have tested this more thoroughly. Yeah, that's a significant issue. Regarding Aspirate: no, there's no issue tracking this. While this could be "fixed" in Aspirate, it didn't seem appropriate as this behavior is confusing apart from it. I most recently encountered this trying to containerize a console application while testing other things. There is no indication If this could be made to work with the issue you shared above resolved, is this something you could take? I think this is going to occur more frequently than you might expect; I specifically encountered it during a push containerize legacy systems. |
Console apps are going to be the only kinds of apps that you hit this with - web and worker SDK apps have the default set correctly. The main reason console apps are a problem here is because there's no distinguishing heuristic/marker between them and a classlib project that I could find. I'm trying to remember why the 'expected' heuristic of |
@John-Leitch I think I remember what the reason was: back when we were doing these defaults, the primary way to publish a container was via a Publish Profile - which required the project to import a set of logic and Tasks called the 'publish SDK'. Web and Worker SDK projects do this already, so they lit up container support easily. However, Console projects couldn't import the Publish SDK easily because of MSBuild ordering/layering constraints. Now that we've mostly moved off of PublishProfile-based support and onto 'normal' MSBuild Target invocation ( |
Currently, the
PublishContainer
Target
is skipped ifEnableSdkContainerSupport
is nottrue
. In some cases this leads to confusing behavior, wherein the publish to container appears successful, with no indication otherwise beyond the lack of an image. In the case of the Microsoft-recommended community tool Aspirate, this is especially problematic as it results in the emitting of deployments that point to non-existent images.This PR attempts to remedy this by updating
Microsoft.NET.Build.Containers.targets
to throw a new error,CONTAINERS0667
, in the event of publishing to container whenEnableSdkContainerSupport
is nottrue
.