- Notifications
You must be signed in to change notification settings - Fork906
feat(agent): implement recreate for devcontainers#17308
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
+1,338 −22
Merged
Changes fromall commits
Commits
Show all changes
13 commits Select commitHold shift + click to select a range
6d18522 feat(agent): implement recreate for devcontainers
mafredri6b08fd1 remove ai comments from test
mafredridb6157e update comments
mafredric214476 oopsie
mafredri486bb3f consistent var naming
mafredri379b262 restructure test
mafredrica791d6 wait medium just in case
mafredri2692e55 mark testdata as generated
mafredri2687bfd allow paralleltest
mafredrib850b07 rename log writer
mafredri6b8ad8c fixup! allow paralleltest
mafredri1d937a0 fixup! fixup! allow paralleltest
mafredrif5e2328 fixup! fixup! fixup! allow paralleltest
mafredriFile filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
1 change: 1 addition & 0 deletions.gitattributes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
3 changes: 2 additions & 1 deletion.github/workflows/typos.toml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
85 changes: 76 additions & 9 deletionsagent/agentcontainers/containers.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -9,6 +9,8 @@ import ( | ||
| "golang.org/x/xerrors" | ||
| "github.com/go-chi/chi/v5" | ||
| "github.com/coder/coder/v2/coderd/httpapi" | ||
| "github.com/coder/coder/v2/codersdk" | ||
| "github.com/coder/quartz" | ||
| @@ -20,9 +22,10 @@ const ( | ||
| getContainersTimeout = 5 * time.Second | ||
| ) | ||
| typeHandler struct { | ||
| cacheDuration time.Duration | ||
| cl Lister | ||
| dccli DevcontainerCLI | ||
| clock quartz.Clock | ||
| // lockCh protects the below fields. We use a channel instead of a mutex so we | ||
| @@ -32,20 +35,26 @@ type devcontainersHandler struct { | ||
| mtime time.Time | ||
| } | ||
| // Option is a functional option forHandler. | ||
| type Option func(*Handler) | ||
| // WithLister sets the agentcontainers.Lister implementation to use. | ||
| // The default implementation uses the Docker CLI to list containers. | ||
| func WithLister(cl Lister) Option { | ||
| return func(ch *Handler) { | ||
| ch.cl = cl | ||
| } | ||
| } | ||
| func WithDevcontainerCLI(dccli DevcontainerCLI) Option { | ||
| return func(ch *Handler) { | ||
| ch.dccli = dccli | ||
| } | ||
| } | ||
| // New returns a new Handler with the given options applied. | ||
| func New(options ...Option) *Handler { | ||
| ch := &Handler{ | ||
| lockCh: make(chan struct{}, 1), | ||
| } | ||
| for _, opt := range options { | ||
| @@ -54,7 +63,7 @@ func New(options ...Option) http.Handler { | ||
| return ch | ||
| } | ||
| func (ch *Handler) List(rw http.ResponseWriter, r *http.Request) { | ||
| select { | ||
| case <-r.Context().Done(): | ||
| // Client went away. | ||
| @@ -80,7 +89,7 @@ func (ch *devcontainersHandler) ServeHTTP(rw http.ResponseWriter, r *http.Reques | ||
| } | ||
| } | ||
| func (ch *Handler) getContainers(ctx context.Context) (codersdk.WorkspaceAgentListContainersResponse, error) { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return codersdk.WorkspaceAgentListContainersResponse{}, ctx.Err() | ||
| @@ -149,3 +158,61 @@ var _ Lister = NoopLister{} | ||
| func (NoopLister) List(_ context.Context) (codersdk.WorkspaceAgentListContainersResponse, error) { | ||
| return codersdk.WorkspaceAgentListContainersResponse{}, nil | ||
| } | ||
| func (ch *Handler) Recreate(w http.ResponseWriter, r *http.Request) { | ||
| ctx := r.Context() | ||
| id := chi.URLParam(r, "id") | ||
| if id == "" { | ||
| httpapi.Write(ctx, w, http.StatusBadRequest, codersdk.Response{ | ||
| Message: "Missing container ID or name", | ||
| Detail: "Container ID or name is required to recreate a devcontainer.", | ||
| }) | ||
| return | ||
| } | ||
| containers, err := ch.cl.List(ctx) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Potential follow-up (non-blocking): I wonder if we should also add an | ||
| if err != nil { | ||
| httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{ | ||
| Message: "Could not list containers", | ||
| Detail: err.Error(), | ||
| }) | ||
| return | ||
| } | ||
| containerIdx := slices.IndexFunc(containers.Containers, func(c codersdk.WorkspaceAgentContainer) bool { | ||
| return c.Match(id) | ||
| }) | ||
| if containerIdx == -1 { | ||
| httpapi.Write(ctx, w, http.StatusNotFound, codersdk.Response{ | ||
| Message: "Container not found", | ||
| Detail: "Container ID or name not found in the list of containers.", | ||
| }) | ||
| return | ||
| } | ||
| container := containers.Containers[containerIdx] | ||
| workspaceFolder := container.Labels[DevcontainerLocalFolderLabel] | ||
| configPath := container.Labels[DevcontainerConfigFileLabel] | ||
| // Workspace folder is required to recreate a container, we don't verify | ||
| // the config path here because it's optional. | ||
| if workspaceFolder == "" { | ||
| httpapi.Write(ctx, w, http.StatusBadRequest, codersdk.Response{ | ||
| Message: "Missing workspace folder label", | ||
| Detail: "The workspace folder label is required to recreate a devcontainer.", | ||
| }) | ||
| return | ||
| } | ||
| _, err = ch.dccli.Up(ctx, workspaceFolder, configPath, WithRemoveExistingContainer()) | ||
| if err != nil { | ||
| httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{ | ||
| Message: "Could not recreate devcontainer", | ||
| Detail: err.Error(), | ||
| }) | ||
| return | ||
| } | ||
| w.WriteHeader(http.StatusNoContent) | ||
| } | ||
2 changes: 1 addition & 1 deletionagent/agentcontainers/containers_internal_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
166 changes: 166 additions & 0 deletionsagent/agentcontainers/containers_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,166 @@ | ||
| package agentcontainers_test | ||
| import ( | ||
| "context" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "testing" | ||
| "github.com/go-chi/chi/v5" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| "golang.org/x/xerrors" | ||
| "github.com/coder/coder/v2/agent/agentcontainers" | ||
| "github.com/coder/coder/v2/codersdk" | ||
| ) | ||
| // fakeLister implements the agentcontainers.Lister interface for | ||
| // testing. | ||
| type fakeLister struct { | ||
| containers codersdk.WorkspaceAgentListContainersResponse | ||
| err error | ||
| } | ||
| func (f *fakeLister) List(_ context.Context) (codersdk.WorkspaceAgentListContainersResponse, error) { | ||
| return f.containers, f.err | ||
| } | ||
| // fakeDevcontainerCLI implements the agentcontainers.DevcontainerCLI | ||
| // interface for testing. | ||
| type fakeDevcontainerCLI struct { | ||
| id string | ||
| err error | ||
| } | ||
| func (f *fakeDevcontainerCLI) Up(_ context.Context, _, _ string, _ ...agentcontainers.DevcontainerCLIUpOptions) (string, error) { | ||
| return f.id, f.err | ||
| } | ||
| func TestHandler(t *testing.T) { | ||
| t.Parallel() | ||
| t.Run("Recreate", func(t *testing.T) { | ||
| t.Parallel() | ||
| validContainer := codersdk.WorkspaceAgentContainer{ | ||
| ID: "container-id", | ||
| FriendlyName: "container-name", | ||
| Labels: map[string]string{ | ||
| agentcontainers.DevcontainerLocalFolderLabel: "/workspace", | ||
| agentcontainers.DevcontainerConfigFileLabel: "/workspace/.devcontainer/devcontainer.json", | ||
| }, | ||
| } | ||
| missingFolderContainer := codersdk.WorkspaceAgentContainer{ | ||
| ID: "missing-folder-container", | ||
| FriendlyName: "missing-folder-container", | ||
| Labels: map[string]string{}, | ||
| } | ||
| tests := []struct { | ||
| name string | ||
| containerID string | ||
| lister *fakeLister | ||
| devcontainerCLI *fakeDevcontainerCLI | ||
| wantStatus int | ||
| wantBody string | ||
| }{ | ||
| { | ||
| name: "Missing ID", | ||
| containerID: "", | ||
| lister: &fakeLister{}, | ||
| devcontainerCLI: &fakeDevcontainerCLI{}, | ||
| wantStatus: http.StatusBadRequest, | ||
| wantBody: "Missing container ID or name", | ||
| }, | ||
| { | ||
| name: "List error", | ||
| containerID: "container-id", | ||
| lister: &fakeLister{ | ||
| err: xerrors.New("list error"), | ||
| }, | ||
| devcontainerCLI: &fakeDevcontainerCLI{}, | ||
| wantStatus: http.StatusInternalServerError, | ||
| wantBody: "Could not list containers", | ||
| }, | ||
| { | ||
| name: "Container not found", | ||
| containerID: "nonexistent-container", | ||
| lister: &fakeLister{ | ||
| containers: codersdk.WorkspaceAgentListContainersResponse{ | ||
| Containers: []codersdk.WorkspaceAgentContainer{validContainer}, | ||
| }, | ||
| }, | ||
| devcontainerCLI: &fakeDevcontainerCLI{}, | ||
| wantStatus: http.StatusNotFound, | ||
| wantBody: "Container not found", | ||
| }, | ||
| { | ||
| name: "Missing workspace folder label", | ||
| containerID: "missing-folder-container", | ||
| lister: &fakeLister{ | ||
| containers: codersdk.WorkspaceAgentListContainersResponse{ | ||
| Containers: []codersdk.WorkspaceAgentContainer{missingFolderContainer}, | ||
| }, | ||
| }, | ||
| devcontainerCLI: &fakeDevcontainerCLI{}, | ||
| wantStatus: http.StatusBadRequest, | ||
| wantBody: "Missing workspace folder label", | ||
| }, | ||
| { | ||
| name: "Devcontainer CLI error", | ||
| containerID: "container-id", | ||
| lister: &fakeLister{ | ||
| containers: codersdk.WorkspaceAgentListContainersResponse{ | ||
| Containers: []codersdk.WorkspaceAgentContainer{validContainer}, | ||
| }, | ||
| }, | ||
| devcontainerCLI: &fakeDevcontainerCLI{ | ||
| err: xerrors.New("devcontainer CLI error"), | ||
| }, | ||
| wantStatus: http.StatusInternalServerError, | ||
| wantBody: "Could not recreate devcontainer", | ||
| }, | ||
| { | ||
| name: "OK", | ||
| containerID: "container-id", | ||
| lister: &fakeLister{ | ||
| containers: codersdk.WorkspaceAgentListContainersResponse{ | ||
| Containers: []codersdk.WorkspaceAgentContainer{validContainer}, | ||
| }, | ||
| }, | ||
| devcontainerCLI: &fakeDevcontainerCLI{}, | ||
| wantStatus: http.StatusNoContent, | ||
| wantBody: "", | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| // Setup router with the handler under test. | ||
| r := chi.NewRouter() | ||
| handler := agentcontainers.New( | ||
| agentcontainers.WithLister(tt.lister), | ||
| agentcontainers.WithDevcontainerCLI(tt.devcontainerCLI), | ||
| ) | ||
| r.Post("/containers/{id}/recreate", handler.Recreate) | ||
| // Simulate HTTP request to the recreate endpoint. | ||
| req := httptest.NewRequest(http.MethodPost, "/containers/"+tt.containerID+"/recreate", nil) | ||
| rec := httptest.NewRecorder() | ||
| r.ServeHTTP(rec, req) | ||
| // Check the response status code and body. | ||
| require.Equal(t, tt.wantStatus, rec.Code, "status code mismatch") | ||
| if tt.wantBody != "" { | ||
| assert.Contains(t, rec.Body.String(), tt.wantBody, "response body mismatch") | ||
| } else if tt.wantStatus == http.StatusNoContent { | ||
| assert.Empty(t, rec.Body.String(), "expected empty response body") | ||
| } | ||
| }) | ||
| } | ||
| }) | ||
| } |
15 changes: 13 additions & 2 deletionsagent/agentcontainers/devcontainer.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -12,6 +12,15 @@ import ( | ||
| "github.com/coder/coder/v2/codersdk" | ||
| ) | ||
| const ( | ||
| // DevcontainerLocalFolderLabel is the label that contains the path to | ||
| // the local workspace folder for a devcontainer. | ||
| DevcontainerLocalFolderLabel = "devcontainer.local_folder" | ||
| // DevcontainerConfigFileLabel is the label that contains the path to | ||
| // the devcontainer.json configuration file. | ||
| DevcontainerConfigFileLabel = "devcontainer.config_file" | ||
| ) | ||
| const devcontainerUpScriptTemplate = ` | ||
| if ! which devcontainer > /dev/null 2>&1; then | ||
| echo "ERROR: Unable to start devcontainer, @devcontainers/cli is not installed." | ||
| @@ -52,8 +61,10 @@ ScriptLoop: | ||
| } | ||
| func devcontainerStartupScript(dc codersdk.WorkspaceAgentDevcontainer, script codersdk.WorkspaceAgentScript) codersdk.WorkspaceAgentScript { | ||
| args := []string{ | ||
| "--log-format json", | ||
mafredri marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| fmt.Sprintf("--workspace-folder %q", dc.WorkspaceFolder), | ||
| } | ||
| if dc.ConfigPath != "" { | ||
| args = append(args, fmt.Sprintf("--config %q", dc.ConfigPath)) | ||
| } | ||
Oops, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
Oops, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.