- Notifications
You must be signed in to change notification settings - Fork1k
fix(agent): fix script filtering for devcontainers#18635
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
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1158,15 +1158,13 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, | ||
| } | ||
| } | ||
| scripts := manifest.Scripts | ||
| if a.containerAPI != nil { | ||
| // Since devcontainer are enabled, remove devcontainer scripts | ||
| // from the main scripts list to avoid showing an error. | ||
| scripts, _ = agentcontainers.ExtractDevcontainerScripts(manifest.Devcontainers, manifest.Scripts) | ||
| } | ||
| err = a.scriptRunner.Init(scripts, aAPI.ScriptCompleted) | ||
| if err != nil { | ||
| return xerrors.Errorf("init script runner: %w", err) | ||
| } | ||
| @@ -1187,10 +1185,11 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, | ||
| if a.containerAPI != nil { | ||
| a.containerAPI.Init( | ||
| agentcontainers.WithManifestInfo(manifest.OwnerName, manifest.WorkspaceName, manifest.AgentName), | ||
| agentcontainers.WithDevcontainers(manifest.Devcontainers,manifest.Scripts), | ||
| agentcontainers.WithSubAgentClient(agentcontainers.NewSubAgentClientFromAPI(a.logger, aAPI)), | ||
| ) | ||
| _, devcontainerScripts := agentcontainers.ExtractDevcontainerScripts(manifest.Devcontainers, manifest.Scripts) | ||
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. Is there any reason to call this function twice? 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. Mainly I wanted to reduce variable scope to avoid similar issues as introduced in the previous PR. And make it clear that scripts from earlier shouldn't be used here. It's a lightweight call but I can revert it if you prefer. 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. I have no issue with the duplication just wanted to ensure I knew the reason why, all good! | ||
| for _, dc := range manifest.Devcontainers { | ||
| cErr := a.createDevcontainer(ctx, aAPI, dc, devcontainerScripts[dc.ID]) | ||
| err = errors.Join(err, cErr) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -79,15 +79,14 @@ type API struct { | ||
| containersErr error // Error from the last list operation. | ||
| devcontainerNames map[string]bool // By devcontainer name. | ||
| knownDevcontainers map[string]codersdk.WorkspaceAgentDevcontainer // By workspace folder. | ||
| devcontainerLogSourceIDs map[string]uuid.UUID // By workspace folder. | ||
| configFileModifiedTimes map[string]time.Time // By config file path. | ||
| recreateSuccessTimes map[string]time.Time // By workspace folder. | ||
| recreateErrorTimes map[string]time.Time // By workspace folder. | ||
| injectedSubAgentProcs map[string]subAgentProcess // By workspace folder. | ||
| usingWorkspaceFolderName map[string]bool // By workspace folder. | ||
| ignoredDevcontainers map[string]bool // By workspace folder. Tracks three states (true, false and not checked). | ||
| asyncWg sync.WaitGroup | ||
| } | ||
| type subAgentProcess struct { | ||
| @@ -935,12 +934,7 @@ func (api *API) CreateDevcontainer(workspaceFolder, configPath string, opts ...D | ||
| return xerrors.Errorf("devcontainer not found") | ||
| } | ||
| var ( | ||
| ctx = api.ctx | ||
| logger = api.logger.With( | ||
| slog.F("devcontainer_id", dc.ID), | ||
| @@ -950,19 +944,23 @@ func (api *API) CreateDevcontainer(workspaceFolder, configPath string, opts ...D | ||
| ) | ||
| ) | ||
| // Send logs via agent logging facilities. | ||
| logSourceID := api.devcontainerLogSourceIDs[dc.WorkspaceFolder] | ||
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. Review: We were accessing this outside the mutex so I moved it just in case due to change in how we | ||
| if logSourceID == uuid.Nil { | ||
| api.logger.Debug(api.ctx, "devcontainerlog source ID not found, falling back to external log source ID") | ||
| logSourceID = agentsdk.ExternalLogSourceID | ||
| } | ||
| api.asyncWg.Add(1) | ||
| defer api.asyncWg.Done() | ||
| api.mu.Unlock() | ||
| if dc.ConfigPath != configPath { | ||
| logger.Warn(ctx, "devcontainer config path mismatch", | ||
| slog.F("config_path_param", configPath), | ||
| ) | ||
| } | ||
| scriptLogger := api.scriptLogger(logSourceID) | ||
| defer func() { | ||
| flushCtx, cancel := context.WithTimeout(api.ctx, 5*time.Second) | ||
| @@ -981,7 +979,7 @@ func (api *API) CreateDevcontainer(workspaceFolder, configPath string, opts ...D | ||
| upOptions := []DevcontainerCLIUpOptions{WithUpOutput(infoW, errW)} | ||
| upOptions = append(upOptions, opts...) | ||
| _, err:= api.dccli.Up(ctx, dc.WorkspaceFolder, configPath, upOptions...) | ||
| if err != nil { | ||
| // No need to log if the API is closing (context canceled), as this | ||
| // is expected behavior when the API is shutting down. | ||
Uh oh!
There was an error while loading.Please reload this page.