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

fix(envbuilder): RunCacheProbe: remove references to constants.MagicDir #315

Merged
merged 3 commits into from
Aug 14, 2024

Conversation

johnstcn
Copy link
Member

@johnstcn johnstcn commented Aug 14, 2024

Relates to #314

RunCacheProbe is likely to execute without root permissions (e.g. via Terraform provider).
Writing to constants.MagicDir will fail in this case due to lack of permission.
Instead, use buildTimeWorkspaceFolder to write a Dockerfile when compiling from a devcontainer.json.

@johnstcn johnstcn self-assigned this Aug 14, 2024
@@ -1010,7 +1010,7 @@ func RunCacheProbe(ctx context.Context, opts options.Options) (v1.Image, error)
opts.Logger(log.LevelInfo, "No Dockerfile or image specified; falling back to the default image...")
fallbackDockerfile = defaultParams.DockerfilePath
}
buildParams, err = devContainer.Compile(opts.Filesystem, devcontainerDir, constants.MagicDir, fallbackDockerfile, opts.WorkspaceFolder, false, os.LookupEnv)
buildParams, err = devContainer.Compile(opts.Filesystem, devcontainerDir, buildTimeWorkspaceFolder, fallbackDockerfile, opts.WorkspaceFolder, false, os.LookupEnv)
Copy link
Member

Choose a reason for hiding this comment

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

Will this potentially change any of the Docker instructions/"directives"? That could lead to a cache miss if that's the case. Otherwise LGTM.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see a comment in devcontainer.go that mentions featureDir must be reproducible based on the feature name, but I think that's relative to the build context.

In any case, I think it makes sense to beef up the testing around RunCacheProbe outside of a container as a non-root user.

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked into adding some more non-rootful cache probe tests and I then remembered that you actually need the envbuilder binary for this. The infrastructure for all that is already there in the terraform provider tests, so that's probably a better place to do it.

@johnstcn johnstcn merged commit be15d1a into main Aug 14, 2024
4 checks passed
@johnstcn johnstcn deleted the cj/cache-probe-no-magicdir branch August 14, 2024 20:35
johnstcn added a commit that referenced this pull request Aug 15, 2024
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.

2 participants