Skip to content
This repository was archived by the owner on Mar 26, 2025. It is now read-only.

Improve multi-stage image build support. #58

Merged
merged 2 commits into from
Jul 8, 2022
Merged

Improve multi-stage image build support. #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c658af
Improve multi-stage image build support.
tmds Jul 6, 2022
4f7553d
Skip SPA build when DOTNET_RUNNING_IN_CONTAINER.
tmds Jul 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/Angular-CSharp.csproj.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@
<NoDefaultLaunchSettingsFile Condition="'$(ExcludeLaunchSettings)' == 'True'">True</NoDefaultLaunchSettingsFile>
<!--#if (!NoSpaFrontEnd) -->
<SpaRoot>ClientApp\</SpaRoot>
<SpaInstallCommand>npm install</SpaInstallCommand>
<SpaBuildCommand>npm run build</SpaBuildCommand>
<SpaProxyLaunchCommand>npm start</SpaProxyLaunchCommand>
<SpaProxyServerUrl Condition="'$(RequiresHttps)' == 'True'">https://localhost:5002</SpaProxyServerUrl>
<SpaProxyServerUrl Condition="'$(RequiresHttps)' != 'True'">http://localhost:5002</SpaProxyServerUrl>
<SpaProxyLaunchCommand>npm start</SpaProxyLaunchCommand>
<!--#endif -->
<RootNamespace Condition="'$(name)' != '$(name{-VALUE-FORMS-}safe_namespace)'">Company.WebApplication1</RootNamespace>
<ImplicitUsings>enable</ImplicitUsings>
Expand Down Expand Up @@ -46,14 +48,15 @@
</Exec>
<Error Condition="'$(ErrorCode)' != '0'" Text="Node.js is required to build and run this project. To continue, please install Node.js from https://nodejs.org/, and then restart your command prompt or IDE." />
<Message Importance="high" Text="Restoring dependencies using 'npm'. This may take several minutes..." />
<Exec WorkingDirectory="$(SpaRoot)" Command="npm install" />
<Exec WorkingDirectory="$(SpaRoot)" Command="$(SpaInstallCommand)" />
</Target>
<!--/+:cnd:noEmit -->

<Target Name="PublishRunWebpack" AfterTargets="ComputeFilesToPublish">
<!-- As part of publishing, ensure the JS resources are freshly built in production mode -->
<Exec WorkingDirectory="$(SpaRoot)" Command="npm install" />
<Exec WorkingDirectory="$(SpaRoot)" Command="npm run build -- --configuration production" />
<SkipSpaBuild Condition="'$(DOTNET_RUNNING_IN_CONTAINER)' == 'true'">True</SkipSpaBuild>
<Exec WorkingDirectory="$(SpaRoot)" Command="$(SpaInstallCommand)" Condition="'$(SkipSpaBuild)' != 'True'" />
<Exec WorkingDirectory="$(SpaRoot)" Command="$(SpaBuildCommand)" Condition="'$(SkipSpaBuild)' != 'True'" />
Copy link
Member

@javiercn javiercn Jul 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can we use Condition="'$(DOTNET_RUNNING_IN_CONTAINER)' != 'True'" here instead? That will automatically do the right thing here without having to set anything additional.

Copy link
Member Author

Choose a reason for hiding this comment

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

That will break users that have this setup as a single-stage build on an image with .NET and nodejs (#58 (comment)).

I know some customers build .NET SPA apps this way.

If this should work automagically we could skip the SPA build when DOTNET_RUNNING_IN_CONTAINER and nodejs is not in the image.

Copy link
Member Author

Choose a reason for hiding this comment

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

If this should work automagically we could skip the SPA build when DOTNET_RUNNING_IN_CONTAINER and nodejs is not in the image.

Personally, I think there a user won't have trouble setting /p:SkipSpaBuild=true from their multi-stage Dockerfile.
But if you prefer, I'll update to use the combination of DOTNET_RUNNING_IN_CONTAINER and no nodejs.

Copy link
Member

Choose a reason for hiding this comment

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

and no nodejs.

Can you clarify what do you mean by that?

The appeal of DOTNET_RUNNING_IN_CONTAINER is that the container image already sets it, so the user doesn't have to take/make an additional step in their Dockerfile and is one less concept they need to understand.

Copy link
Member Author

Choose a reason for hiding this comment

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

A user that wants to do a single-stage SPA build will have a .NET image (which should have DOTNET_RUNNING_IN_CONTAINER) that also includes nodejs.

If our logic is SkipSpaBuild = DOTNET_RUNNING_IN_CONTAINER == "True", then the single-stage build will not work.

To continue supporting the single-stage SPA build, we need to extend the condition with: and no nodejs.

Copy link
Member

Choose a reason for hiding this comment

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

At that point the user has already created their own container image, I think they can manage to update their csproj file. The number of users in that scenario is far less than the users leveraging our existing images.

Copy link
Member Author

Choose a reason for hiding this comment

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

Before this change, users that wanted to do multi-stage builds had to update their csproj.
With what you suggest, after the change, users that want to do single-stage builds have to update their csproj.

I think it would be nice if both scenarios were possible without requiring updates to the csproj file.

I'm going to apply your suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

With what you suggest, after the change, users that want to do single-stage builds have to update their csproj.

They would have to update their docker image either way to pass an additional flag, wouldn't they? I am not against adding an extra flag, I just want to make sure we reason through the scenarios.

Not all the scenarios are weighted the same, and I lean way more towards making building against our images work without having to pass additional settings than making it easier for people building their own images, as I think the first scenario will be far more common.

Whenever we tell people to do this in their docker image, the additional step only involves adding an extra build step and copying the outputs to the final build, there is no need for them to go and add a flag to their dotnet publish step.


<!-- Include the newly-built files in the publish output -->
<ItemGroup>
Expand Down
11 changes: 7 additions & 4 deletions src/React-CSharp.csproj.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@
<NoDefaultLaunchSettingsFile Condition="'$(ExcludeLaunchSettings)' == 'True'">True</NoDefaultLaunchSettingsFile>
<!--#if (!NoSpaFrontEnd) -->
<SpaRoot>ClientApp\</SpaRoot>
<SpaInstallCommand>npm install</SpaInstallCommand>
<SpaBuildCommand>npm run build</SpaBuildCommand>
<SpaProxyLaunchCommand>npm start</SpaProxyLaunchCommand>
<DefaultItemExcludes>$(DefaultItemExcludes);$(SpaRoot)node_modules\**</DefaultItemExcludes>
<SpaProxyServerUrl Condition="'$(RequiresHttps)' == 'True'">https://localhost:5002</SpaProxyServerUrl>
<SpaProxyServerUrl Condition="'$(RequiresHttps)' != 'True'">http://localhost:5002</SpaProxyServerUrl>
<SpaProxyLaunchCommand>npm start</SpaProxyLaunchCommand>
<!--#endif -->
<RootNamespace Condition="'$(name)' != '$(name{-VALUE-FORMS-}safe_namespace)'">Company.WebApplication1</RootNamespace>
<ImplicitUsings>enable</ImplicitUsings>
Expand Down Expand Up @@ -49,14 +51,15 @@
</Exec>
<Error Condition="'$(ErrorCode)' != '0'" Text="Node.js is required to build and run this project. To continue, please install Node.js from https://nodejs.org/, and then restart your command prompt or IDE." />
<Message Importance="high" Text="Restoring dependencies using 'npm'. This may take several minutes..." />
<Exec WorkingDirectory="$(SpaRoot)" Command="npm install" />
<Exec WorkingDirectory="$(SpaRoot)" Command="$(SpaInstallCommand)" />
</Target>
<!--/+:cnd:noEmit -->

<Target Name="PublishRunWebpack" AfterTargets="ComputeFilesToPublish">
<!-- As part of publishing, ensure the JS resources are freshly built in production mode -->
<Exec WorkingDirectory="$(SpaRoot)" Command="npm install" />
<Exec WorkingDirectory="$(SpaRoot)" Command="npm run build" />
<SkipSpaBuild Condition="'$(DOTNET_RUNNING_IN_CONTAINER)' == 'true'">True</SkipSpaBuild>
<Exec WorkingDirectory="$(SpaRoot)" Command="$(SpaInstallCommand)" Condition="'$(SkipSpaBuild)' != 'True'" />
<Exec WorkingDirectory="$(SpaRoot)" Command="$(SpaBuildCommand)" Condition="'$(SkipSpaBuild)' != 'True'" />

<!-- Include the newly-built files in the publish output -->
<ItemGroup>
Expand Down