- Notifications
You must be signed in to change notification settings - Fork907
feat(agent/agentcontainers): add devcontainers list endpoint#17389
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
feat(agent/agentcontainers): add devcontainers list endpoint#17389
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This change allows listing both predefined and runtime-detecteddevcontainers, as well as showing whether or not the devcontainer isrunning and which container represents it.Fixescoder/internal#478
| @@ -121,12 +148,11 @@ func (api *API) getContainers(ctx context.Context) (codersdk.WorkspaceAgentListC | |||
| select { | |||
| case <-ctx.Done(): | |||
| return codersdk.WorkspaceAgentListContainersResponse{}, ctx.Err() | |||
| default: | |||
| api.lockCh <- struct{}{} | |||
| case api.lockCh <- struct{}{}: | |||
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.
Review: Drive-by bug fix.
| @@ -158,7 +234,7 @@ func (api *API) handleRecreate(w http.ResponseWriter, r *http.Request) { | |||
| return | |||
| } | |||
| containers, err := api.cl.List(ctx) | |||
| containers, err := api.getContainers(ctx) | |||
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.
Review: Drive-by bug fix.
agent/agentcontainers/api.go Outdated
| lockCh chan struct{} | ||
| containers codersdk.WorkspaceAgentListContainersResponse | ||
| mtime time.Time | ||
| devcontainersMu sync.Mutex // Protects following. |
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.
Do we need a separate mutex here? Can we not reuselockCh?
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.
It's not as ergonomic but sure, switched to it 😄
| // Check if the container is running and update the known devcontainers. | ||
| for _, container := range updated.Containers { | ||
| workspaceFolder := container.Labels[DevcontainerLocalFolderLabel] |
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.
thought, non-blocking: Given that aWorkspaceAgentDevcontainer is a superset of aWorkspaceAgentContainer, I wonder if we could instead store the optional devcontainer-specific fields inWorkspaceAgentContainer?
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.
The main issue with that approach is: how do we represent a devcontainer we know about but that has no container? (I.e. either it wasn't started, or it was deleted.)
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.
Gotcha 👍
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.
looks good to me
00b5f56 intomainUh oh!
There was an error while loading.Please reload this page.
This change allows listing both predefined and runtime-detected
devcontainers, as well as showing whether or not the devcontainer is
running and which container represents it.
Fixescoder/internal#478
The intent here is to replace the currently used containers endpoint with this one as it provides more context on devcontainers, including ones that have not been started yet, or removed at some point.