- Notifications
You must be signed in to change notification settings - Fork927
fix!: use devcontainer ID when rebuilding a devcontainer#18604
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
Conversation
This PR replaces the use of the **container** ID with the**devcontainer** ID. This is a breaking change. This allows rebuilding adevcontainer when there is no valid container ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR changes API endpoints and internal references to use the devcontainer ID rather than the container ID. Key changes include updating URL paths, parameter names, and related error messages in both the API and its documentation.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| site/src/modules/resources/AgentDevcontainerCard.tsx | Updated the endpoint URL to use devcontainer.id |
| docs/reference/api/agents.md | Updated the API documentation and parameter names |
| codersdk/workspacesdk/agentconn.go | Changed function signature and URL path for recreating devcontainers |
| codersdk/workspaceagents.go | Refactored the workspace agent recreate method to use devcontainerID |
| coderd/workspaceagents_test.go | Updated tests to align with the new devcontainer ID usage |
| coderd/workspaceagents.go, coderd/coderd.go, coderd/apidoc/swagger.json, coderd/apidoc/docs.go | Modified routing, error messages, and API docs for consistency |
| agent/agentcontainers/api_test.go, agent/agentcontainers/api.go | Adjusted API routes and tests to reflect the devcontainer changes |
agent/agentcontainers/api.go Outdated
| return | ||
| varworkspaceFolderstring | ||
| for_,dc:=rangeapi.knownDevcontainers { | ||
| ifdc.ID.String()==devcontainerID { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Consider refactoring the 'knownDevcontainers' map to be keyed by the devcontainer ID instead of the workspaceFolder. This change can improve lookup clarity and avoid potential key collisions when multiple devcontainers share the same workspace folder.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Except for the minor nit, this looks good to me. A bit more invasive than I imagined but we should be OK and good to get this changed before GA 👍🏻
| ifdc.ID.String()==devcontainerID { | ||
| workspaceFolder=dc.WorkspaceFolder | ||
| break | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Suggestion: This could be simplified by storingdc from the for-loop rather than doing two lookups first ID then workspace folder (which we already know).
| @@ -494,8 +494,8 @@ func (api *API) Routes() http.Handler { | |||
| r.Get("/",api.handleList) | |||
| // TODO(mafredri): Simplify this route as the previous /devcontainers | |||
| // /-route was dropped. We can drop the /devcontainers prefix here too. | |||
| r.Route("/devcontainers",func(r chi.Router) { | |||
| r.Post("/container/{container}/recreate",api.handleDevcontainerRecreate) | |||
| r.Route("/devcontainers/{devcontainer}",func(r chi.Router) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This dawned on me just now, but we could takeworkspaceFolder here instead, which will allow starting any non-started devcontainer even if it doesn't exist. 😅 We can table this for now though as implementing it would be preemptive and more thought behind it would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oooh that could be good to add in the future.
| @@ -626,7 +638,7 @@ func TestAPI(t *testing.T) { | |||
| for i := range tt.wantStatus { | |||
| // Simulate HTTP request to the recreate endpoint. | |||
| req := httptest.NewRequest(http.MethodPost, "/devcontainers/container/"+tt.containerID+"/recreate", nil). | |||
| req := httptest.NewRequest(http.MethodPost, "/devcontainers/"+tt.devcontainerID+"/recreate", nil). | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is a much more sensible URL 😍
f2d229e intomainUh oh!
There was an error while loading.Please reload this page.
This PR replaces the use of thecontainer ID with thedevcontainer ID. This is a breaking change. This allows rebuilding a devcontainer when there is no valid container ID.