- Notifications
You must be signed in to change notification settings - Fork913
feat: added include_deleted to getWorkspaceByOwnerAndName#2164
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
relates to#1955
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.
Nice! Just a couple of things:
- I think we don't need to actually check if the
include_deletedparameter is a valid boolean; just checking if it is equal to the stringtrueshould be enough here. I know this is taken from existing code though, so I'm willing to defer to others on this. - Would it be possible to add a unit test for this new behaviour in
workspace_test.go? I'm up for pairing on this with you if you'd like!
Uh oh!
There was an error while loading.Please reload this page.
| workspace, err = api.Database.GetWorkspaceByOwnerIDAndName(r.Context(), database.GetWorkspaceByOwnerIDAndNameParams{ | ||
| OwnerID: owner.ID, | ||
| Name: workspaceName, | ||
| Deleted: includeDeleted, |
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 this works as intended. The param here in the SQL matches fordeleted = $includeDeleted.
So if you set this to true, the workspace will not show if the workspace is deleted. We will need to update the SQL too, or just always return deleted workspaces by default.
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.
See how it's handled here:
Lines 57 to 62 in454aea7
| ifworkspace.Deleted&&!showDeleted { | |
| httpapi.Write(rw,http.StatusGone, httpapi.Response{ | |
| Message:fmt.Sprintf("Workspace %q was deleted, you can view this workspace by specifying '?deleted=true' and trying again.",workspace.ID.String()), | |
| }) | |
| return | |
| } |
I think thedeleted query param is kinda annoying as you have to know the state of the workspace to query it.
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 Hmm, testing it in the UI seems to work.
- if
include_deletedis false, then we never enter this code block (becauseerrors.Is(err, sql.ErrNoRows)is false - otherwise we enter this code block and try to query for a deleted workspace
In either case, if we get nothing back, we then go into thehttpapi.Forbidden(rw) block below. I agree editing the SQL would also work. I just didn't know what other ramifications that would have.
Let me know if I'm misunderstanding your comment!
@johnstcn |
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 sure why this tagged the frontend for review but I'm approving to unblock you on that front!
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.
Just some drive-by code style suggestions, looks good to me!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
defining vars in the scope of conditionalCo-authored-by: Mathias Fredriksson <mafredri@gmail.com>
avoid newlineCo-authored-by: Mathias Fredriksson <mafredri@gmail.com>
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
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 good to me! Thanks Kira!
Uh oh!
There was an error while loading.Please reload this page.
| @@ -83,6 +83,7 @@ | |||
| "workspaceapp", | |||
| "workspaceapps", | |||
| "workspacebuilds", | |||
| "workspacename", | |||
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.
👍
shortening test nameCo-authored-by: Cian Johnston <cian@coder.com>
* feat: added include_deletedrelates to#1955* Update coderd/workspaces.godefining vars in the scope of conditionalCo-authored-by: Mathias Fredriksson <mafredri@gmail.com>* Update coderd/workspaces.goavoid newlineCo-authored-by: Mathias Fredriksson <mafredri@gmail.com>* Update coderd/workspaces.goCo-authored-by: Mathias Fredriksson <mafredri@gmail.com>* PR feedback* wrote test, added type* Update coderd/workspaces_test.goshortening test nameCo-authored-by: Cian Johnston <cian@coder.com>* taking out api.ts change for now* casingCo-authored-by: Mathias Fredriksson <mafredri@gmail.com>Co-authored-by: Cian Johnston <cian@coder.com>
relates to#1955
I'm working on#1955: a ticket that will show deleted workspaces in our UI.
@f0ssel made some helpful changes to
getWorkspacesin#2095 to help support the aforementioned UI change.However,@BrunoQuaresma recently updated the Workspace page so that it no longer uses the
getWorkspaceroute, instead favoringgetWorkspaceByOwnerAndName.This PR attempts to add an
include_deletedparam togetWorkspaceByOwnerAndNameso I can continue work on#1955. I've never touched Go before so please let me know what I've broken 🧹P.S. It would be great to generate a type for this addition but idk how to do that.