Skip to content
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

Feature/fix compose multi stage builds #1530

Merged

Conversation

bkneis
Copy link
Contributor

@bkneis bkneis commented Jan 9, 2025

Fixes - #1433

The issue was that during our build pipeline we assumed an Image was used instead of a dockerfile, resulting in a FROM empty string AS final being added. The empty string came from the compose's devcontainer image name, which is nil since we are building the devcontainer.

The fix was to assign the dockerFileContents to the read docker file if no modifications were made. Looking at pkg/devcontainer/parse.go line 223 we perform a check on the target and potentially return an empty string, as the dockerfile does not need to be modified. In this case we need to simply assign dockerFileContents to the variable containing the original dockerfile (line 450 in pkg/devcontainer/compose.go).

I also added 2 examples, one for multi stage builds and a compose. You can use these to validate the fix by seeing the OP's error running devpod up examples/compose then running the same with this PR's build.

@bkneis bkneis requested a review from a team January 9, 2025 10:31
Copy link
Contributor

@janekbaraniewski janekbaraniewski left a comment

Choose a reason for hiding this comment

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

Awesome, just few nitpicks 😄 really like what you did with including more comments in code handling compose configuration!

I was thinking maybe we could also extract some of those pieces of code to smaller functions (like readOriginalDockerfile etc) so that this buildAndExtendDockerCompose function becomes a bit smaller and easier to grasp and also self-documents better. wdyt?

.env Outdated Show resolved Hide resolved
examples/build-multi-stage/Dockerfile Outdated Show resolved Hide resolved
examples/compose/Dockerfile Outdated Show resolved Hide resolved
@bkneis
Copy link
Contributor Author

bkneis commented Jan 9, 2025

Awesome, just few nitpicks 😄 really like what you did with including more comments in code handling compose configuration!

I was thinking maybe we could also extract some of those pieces of code to smaller functions (like readOriginalDockerfile etc) so that this buildAndExtendDockerCompose function becomes a bit smaller and easier to grasp and also self-documents better. wdyt?

yes for sure that sounds good, I'll try that now

@pascalbreuninger pascalbreuninger merged commit fc0e5eb into loft-sh:main Jan 9, 2025
17 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants