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

Conversation

tmds
Copy link
Member

@tmds tmds commented Jul 6, 2022

Make it possible to opt-out of the nodejs frontend build when the .NET backend gets build.

This allows to build the frontend and backend in separate build stages.

Fixes dotnet/aspnetcore#41275.

@javiercn @richlander ptal.

Make it possible to opt-out of the nodejs frontend build when the
.NET backend gets build.

This allows to build the frontend and backend in separate build stages.
@tmds
Copy link
Member Author

tmds commented Jul 6, 2022

I'm not using DOTNET_RUNNING_IN_CONTAINER at the moment (suggestion in dotnet/aspnetcore#41275 (comment)) because the build container may have nodejs installed for a single-stage build.
The user needs to set SkipSpaBuild=true in the multi-stage Dockerfile.

I'll adjust based on your feedback.

@richlander
Copy link
Member

I looked at the changes. I'm not the best person to review them.

@tmds
Copy link
Member Author

tmds commented Jul 8, 2022

@javiercn can you take a look?

<Exec WorkingDirectory="$(SpaRoot)" Command="npm install" />
<Exec WorkingDirectory="$(SpaRoot)" Command="npm run build -- --configuration production" />
<Exec WorkingDirectory="$(SpaRoot)" Command="$(SpaInstallCommand)" Condition="'$(SkipSpaBuild)' != 'True'" />
<Exec WorkingDirectory="$(SpaRoot)" Command="$(SpaBuildCommand)" Condition="'$(SkipSpaBuild)' != 'True'" />
Copy link
Member

@javiercn javiercn Jul 8, 2022

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.

@javiercn javiercn merged commit a20ae68 into dotnet:main Jul 8, 2022
@javiercn
Copy link
Member

javiercn commented Jul 8, 2022

Thanks for the contribution.

javiercn added a commit that referenced this pull request Jul 8, 2022
javiercn added a commit that referenced this pull request Jul 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update spa templates to facilitate multi-stage docker builds
3 participants