-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: deprecate codersdk.AITaskPromptParameterName and reduce usage #20501
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
483371e to
b3dd0e7
Compare
| -- Consider all tasks, deleting a task does not turn the | ||
| -- workspace into a non-task workspace. | ||
| tasks.workspace_id = workspaces.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.
Review: This here breaks with un-patched sqlc because it puts tasks.owner_id in global scope even though it's not selected.
Should we fork sqlc for now?
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.
I'm fine with forking sqlc temporarily while an upstream bug is filed. I wonder if there's also the possibility of selecting id, workspace_id from tasks in a CTE and doing the join on that?
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.
Can you drop the original bugged sql query? I'm curious to see what the failing/bugged query looked like.
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.
@Emyrk This edit IS the bugged query, but check the added testdata in coder/sqlc#1 to see more.
| }). | ||
| WithAgent(). | ||
| WithTask(database.TaskTable{ | ||
| Prompt: prompt, | ||
| }, nil). | ||
| Do() | ||
|
|
||
| return build.Task |
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.
❤️
| r.Route("/aitasks", func(r chi.Router) { | ||
| r.Use(apiKeyMiddleware) | ||
| r.Get("/prompts", api.aiTasksPrompts) | ||
| }) |
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.
I'm potentially concerned about this given our "one minor release" backward compat guarantee. Maybe mark this endpoint deprecated instead?
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.
Was this ever used by anything other than the frontend? Considering the route is experimental and you can't run an old coderd with a newer frontend, I imagined this should be fine.
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.
Not that I'm aware, no. I guess we can remove it with the provisio that this was always under /api/experimental.
Refs coder/sqlc#1 Unblocks #20501 Upstream sqlc-dev/sqlc#4159
Refs coder/sqlc#1 Unblocks #20501 Upstream sqlc-dev/sqlc#4159
Refs coder/sqlc#1 Unblocks #20501 Upstream sqlc-dev/sqlc#4159
Refs coder/sqlc#1 Unblocks #20501 Upstream sqlc-dev/sqlc#4159
Refs coder/sqlc#1 Unblocks #20501 Upstream sqlc-dev/sqlc#4159
Refs coder/sqlc#1 Unblocks #20501 Upstream sqlc-dev/sqlc#4159
b3dd0e7 to
e592af3
Compare
Depends on coder/sqlc#1
Fixes coder/internal#979
Updates coder/internal#973
Notes:
sqlc, without the patch it incorrectly identifies ambigious columns.queries/workspaces.sql:401:13: column reference "owner_id" is ambiguous(false positive)