- Notifications
You must be signed in to change notification settings - Fork1.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 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.
Conversation
483371e tob3dd0e7Compare| -- 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 selectingid, 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 incoder/sqlc#1 to see more.
| }). | ||
| WithAgent(). | ||
| WithTask(database.TaskTable{ | ||
| Prompt:prompt, | ||
| },nil). | ||
| Do() | ||
| returnbuild.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.
Refscoder/sqlc#1Unblocks#20501Upstreamsqlc-dev/sqlc#4159
Refscoder/sqlc#1Unblocks#20501Upstreamsqlc-dev/sqlc#4159
Refscoder/sqlc#1Unblocks#20501Upstreamsqlc-dev/sqlc#4159
Refscoder/sqlc#1Unblocks#20501Upstreamsqlc-dev/sqlc#4159
Refscoder/sqlc#1Unblocks#20501Upstreamsqlc-dev/sqlc#4159
b3dd0e7 toe592af3Compare859e94d intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Depends oncoder/sqlc#1
Fixescoder/internal#979
Updatescoder/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)