fix: use correct ref and path for features when useBuildContexts is true

[mafredri]

This PR fixes an unused functionality for using build context for dev container features that was behind a boolean flag.

The functionality can be enabled like this:

diff --git envbuilder.go envbuilder.go
index 7ea7791..b8c847a 100644
--- envbuilder.go
+++ envbuilder.go
@@ -290,7 +290,7 @@ func Run(ctx context.Context, options Options) error {
                    options.Logger(notcodersdk.LogLevelInfo, "No Dockerfile or image specified; falling back to the default image...")
                    fallbackDockerfile = defaultParams.DockerfilePath
                }
-               buildParams, err = devContainer.Compile(options.Filesystem, devcontainerDir, MagicDir, fallbackDockerfile, options.WorkspaceFolder, false)
+               buildParams, err = devContainer.Compile(options.Filesystem, devcontainerDir, MagicDir, fallbackDockerfile, options.WorkspaceFolder, true)
                if err != nil {
                    return fmt.Errorf("compile devcontainer.json: %w", err)
                }

It's a bit unclear to me whether or not we will need to use this code-path. It depends if feature files should be available in the image or not, in which case the RUN --mount would need to become a COPY. Since I investigated this though, I decided to push the fix as well.

[aaronlehmann]

I'm the one who added this code path. I was using it externally to generate a Dockerfile from envbuilder. This PR broke things rather badly for me: all features fail with

COPY --from=[ghcr.io/devcontainers/features/go](http://ghcr.io/devcontainers/features/go): no stage or image found with that name

Besides not working, this new implementaiton seems wrong because it ignores the tag specified with the feature.

Would it be okay to revert this?

[mtojek]

@aaronlehmann Can you elaborate a bit more on This PR broke things rather badly for me: all features fail with? What are you trying to achieve? Probably some path is not covered with unit tests hence we didn't catch it.

Besides not working, this new implementaiton seems wrong because it ignores the tag specified with the feature.

Ack 👍 A failing sample would be nice.

[aaronlehmann]

This code path is outside of envbuilder: I import the devcontainer package and use it to parse devcontainer.json and produce a Dockerfile. The code path is covered by unit tests, but the unit tests were changed in this PR to expect the new output.

I realize that asking you to maintain a code path you don't actually use is a big ask. I was hoping to avoid maintaining an envbuilder fork, and the devcontainer package does 99% of what I want, but wasn't exactly right when it comes to features (in my situation, they aren't extracted within the build environment). I can go the fork route if that turns out to be a better plan.

[mafredri]

@aaronlehmann could you share a diff between the Dockerfile this used to produce and the new one? I didn't quite catch what it is that broke in your use case. IMO the goal is to be truthful to the spec so if we diverged where we didn't before, happy to look into it.

[aaronlehmann]

Diff from before this change to after this change:

@@ -1,5 +1,5 @@
 FROM scratch AS envbuilder_feature_go
-COPY --from=go / /
+COPY --from=ghcr.io/devcontainers/features/go / /

 FROM my.registry.host:7002/devcontainers/base/rolling:release
 USER root
@@ -10,9 +10,9 @@

 USER root
 # Go 1.3.0 - Installs Go and common Go utilities. Auto-detects latest version and installs needed dependencies.
-WORKDIR /envbuilder-features/go
+WORKDIR /tmp/devcontainer-build-3017770787/features/go-10774d69
 ENV GOPATH=/go
 ENV GOROOT=/usr/local/go
 ENV PATH=/usr/local/go/bin:/go/bin:${PATH}
-RUN --mount=type=bind,from=envbuilder_feature_go,target=/envbuilder-features/go,rw GOLANGCILINTVERSION="latest" VERSION="1.22.3" _CONTAINER_USER="coder" _REMOTE_USER="coder" ./install.sh
+RUN --mount=type=bind,from=envbuilder_feature_go,target=/tmp/devcontainer-build-3017770787/features/go-10774d69,rw GOLANGCILINTVERSION="latest" VERSION="1.22.3" _CONTAINER_USER="coder" _REMOTE_USER="coder" ./install.sh
 USER coder

With my builder, the new Dockerfile fails with

COPY --from=ghcr.io/devcontainers/features/go: no stage or image found with that name

I provide build contexts with names using the keys in buildParams.FeatureContexts (go in this case). But the --from is now looking for ghcr.io/devcontainers/features/go rather than go. I'm not sure if the intent is for the builder to get this image straight from the registry rather than from a build context, but if it is, notice that the tag is missing from the reference. My devcontainer.json specifies :1:

  "features": {
    "ghcr.io/devcontainers/features/go:1": {
      "version": "1.22.3"
    }
  }

[aaronlehmann]

BTW, the post-change code seems to work correctly if I add the build context with the name ghcr.io/devcontainers/features/go rather than go. So perhaps we just need to update FeatureContexts to have the correct keys?

diff --git a/devcontainer/devcontainer.go b/devcontainer/devcontainer.go
index 0454440..f7d97de 100644
--- a/devcontainer/devcontainer.go
+++ b/devcontainer/devcontainer.go
@@ -295,7 +295,7 @@ func (s *Spec) compileFeatures(fs billy.Filesystem, devcontainerDir, scratchDir
                }
                featureDirectives = append(featureDirectives, directive)
                if useBuildContexts {
-                       featureContexts[featureName] = featureDir
+                       featureContexts[featureRef] = featureDir
                        lines = append(lines, fromDirective)
                }
        }

[aaronlehmann]

Opened a PR for this here: https://github.com/coder/envbuilder/pull/243

[aaronlehmann]

Another unfortunate side effect of this PR is that it produces Dockerfiles with nondeterministic paths, i.e.:

WORKDIR /tmp/devcontainer-build-1594228015/features/go-10774d69

This means Dockerfiles involving features can't be cached.

Any concerns with switching back to the old target paths (or something else that's deterministic)?

[aaronlehmann]

I opened #247 to make the target path determinstic - feel free to discuss over there.