Skip to content

[devcontainer] Added shellenv to dockerfile template #1036

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 22, 2023
+12 −4
Merged

[devcontainer] Added shellenv to dockerfile template #1036

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f580719
Added shellenv to dockerfile template
mohsenari May 20, 2023
7e9abd0
Fix typo
mohsenari May 20, 2023
b17e62e
Added conditional last line for dockerfile tmpl
mohsenari May 22, 2023
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
8 changes: 6 additions & 2 deletions internal/boxcli/generate/devcontainer_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,12 @@ type vscode struct {
Extensions []string `json:"extensions"`
}

type dockerfileData struct {
IsDevcontainer bool
}

// CreateDockerfile creates a Dockerfile in path and writes devcontainerDockerfile.tmpl's content into it
func CreateDockerfile(tmplFS embed.FS, path string) error {
func CreateDockerfile(tmplFS embed.FS, path string, isDevcontainer bool) error {
// create dockerfile
file, err := os.Create(filepath.Join(path, "Dockerfile"))
if err != nil {
Expand All @@ -48,7 +52,7 @@ func CreateDockerfile(tmplFS embed.FS, path string) error {
tmplName := "devcontainerDockerfile.tmpl"
t := template.Must(template.ParseFS(tmplFS, "tmpl/"+tmplName))
// write content into file
return t.Execute(file, nil)
return t.Execute(file, &dockerfileData{IsDevcontainer: isDevcontainer})
}

// CreateDevcontainer creates a devcontainer.json in path and writes getDevcontainerContent's output into it
Expand Down
4 changes: 2 additions & 2 deletions internal/impl/devbox.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ func (d *Devbox) GenerateDevcontainer(force bool) error {
redact.Safe(filepath.Base(devContainerPath)), err)
}
// generate dockerfile
err = generate.CreateDockerfile(tmplFS, devContainerPath)
err = generate.CreateDockerfile(tmplFS, devContainerPath, true /* isDevcontainer */)
if err != nil {
return redact.Errorf("error generating dev container Dockerfile in <project>/%s: %w",
redact.Safe(filepath.Base(devContainerPath)), err)
Expand All @@ -392,7 +392,7 @@ func (d *Devbox) GenerateDockerfile(force bool) error {
}

// generate dockerfile
return errors.WithStack(generate.CreateDockerfile(tmplFS, d.projectDir))
return errors.WithStack(generate.CreateDockerfile(tmplFS, d.projectDir, false /* isDevcontainer */))
}

func (d *Devbox) PrintEnvrcContent(w io.Writer) error {
Expand Down
4 changes: 4 additions & 0 deletions internal/impl/tmpl/devcontainerDockerfile.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,8 @@ WORKDIR /code
RUN sudo chown $DEVBOX_USER:root /code
COPY devbox.json devbox.json
RUN devbox install
{{if .IsDevcontainer}}
RUN devbox shellenv --init-hook >> ~/.profile
{{- else}}
CMD ["devbox", "shell"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This line seems redundant

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@apgrucza true. But the problem is we use this template file for both devcontainer setup and for general docker setup.
I think the best way is to remove the CMD and keep shellenv for devcontainer and vice versa for devbox generate dockerfile.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but maybe the same change would be beneficial for the general Dockerfile too?

{{- end}}