- Notifications
You must be signed in to change notification settings - Fork1.1k
refactor(coderd): drop sidebar app constraint and simplify provisionerdserver for tasks#20591
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
refactor(coderd): drop sidebar app constraint and simplify provisionerdserver for tasks#20591
Uh oh!
There was an error while loading.Please reload this page.
Conversation
cbaeff0 toef35c8dCompare| expectHasAiTask:false, | ||
| expectUsageEvent:false, | ||
| expectHasAiTask:true, | ||
| expectUsageEvent:true,// We no longer reset hasAITask if the app doesn't exist. |
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: Do we want this (send usage event) to be false here or not? Essentially this is a task, but invalid app config means it can't really run.
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.
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.
For now I restored the previous behavior of resettinghasAITask if it's an unknown app ID. This change restores both of these booleans to false.
We can revisit if deemed necessary.
647b226 to2d2a5b5Compare| warnUnknownTaskAppID=true | ||
| } | ||
| id,err:=uuid.Parse(appID) | ||
| iferr!=nil { | ||
| returnxerrors.Errorf("parse app id: %w",err) | ||
| } | ||
| taskAppID= uuid.NullUUID{UUID:id,Valid:true} | ||
| agentID,ok:=agentIDByAppID[appID] | ||
| taskAgentID= uuid.NullUUID{UUID:agentID,Valid:ok} | ||
| } | ||
| // This is a hacky workaround for the issue with tasks 'disappearing' on stop: | ||
| // reuse has_ai_task and sidebar_app_id from the previous build. | ||
| // This workaround should be removed as soon as possible. | ||
| ifworkspaceBuild.Transition==database.WorkspaceTransitionStop&&workspaceBuild.BuildNumber>1 { | ||
| ifprevBuild,err:=s.Database.GetWorkspaceBuildByWorkspaceIDAndBuildNumber(ctx, database.GetWorkspaceBuildByWorkspaceIDAndBuildNumberParams{ | ||
| WorkspaceID:workspaceBuild.WorkspaceID, | ||
| BuildNumber:workspaceBuild.BuildNumber-1, | ||
| });err==nil { | ||
| hasAITask=prevBuild.HasAITask.Bool | ||
| taskAppID=prevBuild.AITaskSidebarAppID | ||
| warnUnknownTaskAppID=false | ||
| s.Logger.Debug(ctx,"task workaround: reused has_ai_task and app_id from previous build to keep track of task", | ||
| slog.F("job_id",job.ID.String()), | ||
| slog.F("build_number",prevBuild.BuildNumber), | ||
| slog.F("workspace_id",workspace.ID), | ||
| slog.F("workspace_build_id",workspaceBuild.ID), | ||
| slog.F("transition",string(workspaceBuild.Transition)), | ||
| slog.F("sidebar_app_id",taskAppID.UUID), | ||
| slog.F("has_ai_task",hasAITask), | ||
| ) |
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.
❤️
Uh oh!
There was an error while loading.Please reload this page.
DanielleMaywood left a comment
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.
All good from my end
2d2a5b5 toe5cee2cComparea6b0eae intomainUh oh!
There was an error while loading.Please reload this page.
Updatescoder/internal#973
Updatescoder/internal#974