Skip to content

ensure that single-rid container publishes populate all container information #48584

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

ensure that single-rid container publishes populate all container information #48584

wants to merge 4 commits into from

Conversation

baronfel
Copy link
Member

@baronfel baronfel commented Apr 18, 2025 •
edited
Loading

Fixes #48579

This is a proposed change to the container targets to ensure that for single-rid containers the existing ComputecontainerConfig contract holds.

NOTE: we need to add a test verifying that all the run-time-related information is present for single-rid builds after calling the ComputeContainerConfig target so that we don't regress this in the future.

For a single-RID container publish:
image
Here you can see that by the time ComputeContainerConfig is completed the execution-time properties have been set.

For a multi-RID container publish, the 'outer' parts are computed:
image

And then the per-RID parts are just-in-time called before each RID's container publish:
image

@Copilot Copilot AI review requested due to automatic review settings April 18, 2025 21:17
Copilot
Copilot AI reviewed Apr 18, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)
  • src/Containers/packaging/build/Microsoft.NET.Build.Containers.targets: Language not supported

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Containers Related to dotnet SDK containers functionality untriaged Request triage from a team member labels Apr 18, 2025
@surayya-MS surayya-MS requested a review from a team as a code owner April 24, 2025 20:14
@surayya-MS
Copy link
Member

surayya-MS commented Apr 24, 2025 •
edited
Loading

NOTE: we need to add a test verifying that all the run-time-related information is present for single-rid builds after calling the ComputeContainerConfig target so that we don't regress this in the future.

There are only 2 things that we want to verify are present from _ComputeContainerExecutionArgs after calling ComputeContainerConfig - ContainerUser and ContainerAppCommand.
We already had tests that verify those (CanComputeContainerUser and CanDeferContainerAppCommand) but for calling _ComputeContainerExecutionArgs target not ComputeContainerConfig. I changed the target in those tests.

CanComputeContainerUser test brought to light another bug.
In order to set this property we use _ContainerIsUsingMicrosoftDefaultImages:

sdk/src/Containers/packaging/build/Microsoft.NET.Build.Containers.targets

Lines 198 to 199 in b16dd92

<_ContainerIsUsingMicrosoftDefaultImages Condition="'$(ContainerBaseImage)' == ''">true</_ContainerIsUsingMicrosoftDefaultImages>
<_ContainerIsUsingMicrosoftDefaultImages Condition="'$(ContainerBaseImage)' != ''">false</_ContainerIsUsingMicrosoftDefaultImages>

Here ContainerBaseImage is suposed to be the one defined by the user, before ComputeDotnetBaseImageAndTag task computes it. Previously it was defined inside ComputeContainerBaseImage target:
https://github.com/dotnet/sdk/pull/47424/files#diff-a5f4ada40b043c8a36b7a1d56148dcfd0abcfe13fa82dbfb456f7c43bebedff7L45
So, I introduced new prop _InitialContainerBaseImage to fix this.

@surayya-MS
Copy link
Member

The failing tests are from web assembly, unrelated to the changes in this PR.

@baronfel baronfel force-pushed the restore-computecontainerconfig-data-for-single-rid-containers branch from 4641973 to 7f0f696 Compare April 25, 2025 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@surayya-MS surayya-MS surayya-MS approved these changes

Assignees
No one assigned
Labels
Area-Containers Related to dotnet SDK containers functionality untriaged Request triage from a team member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Containers targets need to keep 'execution time' data ordering consistent for single-RID publishes
2 participants
@baronfel @surayya-MS