- Notifications
You must be signed in to change notification settings - Fork1k
feat: add shared filter to workspaces query#19807
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
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 great! could we add some tests that actually hit the database tho?
aslilac left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 don't think these tests are actually testing much right now, sinceasRequestOption didn't get wired up but they're still passing. 😅
nvm, I didn't see that they're actually failing
codersdk/workspaces.go Outdated
| // FilterQuery supports a raw filter query string | ||
| FilterQuerystring`json:"q,omitempty"` | ||
| // Shared is a whether the workspace is shared with any users or groups | ||
| Sharedbool`json:"shared" typescript:"-"` |
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.
WorkspaceFilterhas anasRequestOptionmethod that also needs to be updated. that's where theqparam actually gets serialized.this field probably needs to be
*boolso that you can setshared:falseas distinct from "noshared:filter"
| // FilterQuery supports a raw filter query string | ||
| FilterQuerystring`json:"q,omitempty"` |
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.
nit: I think this being last was intentional since it doesn't quite "fit in", and that should be preserved
Yeah I'm still working on it, I'll re-request review when it's ready |
c0dc8e8 tobf4b351Compare| workspaces,err:=client.Workspaces(ctx, codersdk.WorkspaceFilter{ | ||
| Shared:ptr.Ref(true), | ||
| }) | ||
| require.NoError(t,err,"fetch workspaces") | ||
| require.Equal(t,1,workspaces.Count,"expected only one workspace") |
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.
since there's only one workspace here, could we also run this withShared: ptr.Ref(false) and check the length to be 0?
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 am creating two workspaces if you look at the setup
| workspaces,err:=client.Workspaces(ctx, codersdk.WorkspaceFilter{ | ||
| Shared:ptr.Ref(true), | ||
| }) | ||
| require.NoError(t,err,"fetch workspaces") | ||
| require.Equal(t,1,workspaces.Count,"expected only one workspace") | ||
| require.Equal(t,workspaces.Workspaces[0].ID,sharedWorkspace.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.
and perhaps the same thing here for the groups case
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.
Same as above, a second workspace is being created but not shared
e6b04d1 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Adds a
shared:<boolean>search query to the/workspaces [get]endpointCleanShot.2025-09-15.at.11.13.25.mp4
Closescoder/internal#972