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.
This seems like the right approach and has all of the necessary pieces 👍
I think the question here is not a valid objection to this change. We want this feature 😆
Can you add a test?
@@ -268,6 +268,10 @@ private static void EvaluateProject(OutputContext output, ProjectServiceBuilder | |||
project.Frameworks.AddRange(sharedFrameworks.Select(s => new Framework(s))); | |||
output.WriteDebugLine($"Found shared frameworks: {string.Join(", ", sharedFrameworks)}"); | |||
|
|||
// determine container base image | |||
project.ContainerInfo!.BaseImageName = projectInstance.GetPropertyValue("ContainerBaseImage"); |
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 think having a null check here for project.ContainerInfo is probably better.
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.
Need the property here, should I create it?
new ContainerInfo() { UseMultiphaseDockerfile = false, };
@@ -268,6 +268,15 @@ private static void EvaluateProject(OutputContext output, ProjectServiceBuilder | |||
project.Frameworks.AddRange(sharedFrameworks.Select(s => new Framework(s))); | |||
output.WriteDebugLine($"Found shared frameworks: {string.Join(", ", sharedFrameworks)}"); | |||
|
|||
// determine container base image | |||
if (project.ContainerInfo == null) |
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 here?
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.
See @jkotalik comment : #447 (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.
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.
@davidfowl I agree and it appears that it is the only entry point. However, @jkotalik is suggesting that there should be a null check, if it is (although it shoudn't be) we must create the instance.
previously I was just setting the values - project.ContainerInfo!.BaseImageName
I had a bunch of small nits, I just went ahead and committed them to the branch |
Fixes #240