- Notifications
You must be signed in to change notification settings - Fork928
Show workspace name in WorkspaceBuildStats component#1933
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.
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -21,13 +21,7 @@ import ( | ||
| func (api *API) workspaceBuild(rw http.ResponseWriter, r *http.Request) { | ||
| workspaceBuild := httpmw.WorkspaceBuildParam(r) | ||
| workspace := httpmw.WorkspaceParam(r) | ||
| if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceWorkspace. | ||
| InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { | ||
| @@ -42,7 +36,7 @@ func (api *API) workspaceBuild(rw http.ResponseWriter, r *http.Request) { | ||
| return | ||
| } | ||
| httpapi.Write(rw, http.StatusOK, convertWorkspaceBuild(workspace,workspaceBuild, job)) | ||
| } | ||
| func (api *API) workspaceBuilds(rw http.ResponseWriter, r *http.Request) { | ||
| @@ -101,7 +95,7 @@ func (api *API) workspaceBuilds(rw http.ResponseWriter, r *http.Request) { | ||
| }) | ||
| return | ||
| } | ||
| apiBuilds = append(apiBuilds, convertWorkspaceBuild(workspace,build, job)) | ||
| } | ||
| httpapi.Write(rw, http.StatusOK, apiBuilds) | ||
| @@ -139,7 +133,7 @@ func (api *API) workspaceBuildByName(rw http.ResponseWriter, r *http.Request) { | ||
| return | ||
| } | ||
| httpapi.Write(rw, http.StatusOK, convertWorkspaceBuild(workspace,workspaceBuild, job)) | ||
| } | ||
| func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { | ||
| @@ -307,7 +301,8 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { | ||
| return | ||
| } | ||
| httpapi.Write(rw, http.StatusCreated, | ||
| convertWorkspaceBuild(workspace, workspaceBuild, provisionerJob)) | ||
| } | ||
| func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Request) { | ||
| @@ -432,19 +427,26 @@ func (api *API) workspaceBuildState(rw http.ResponseWriter, r *http.Request) { | ||
| _, _ = rw.Write(workspaceBuild.ProvisionerState) | ||
| } | ||
| func convertWorkspaceBuild( | ||
| workspace database.Workspace, | ||
| workspaceBuild database.WorkspaceBuild, | ||
| job database.ProvisionerJob) codersdk.WorkspaceBuild { | ||
| //nolint:unconvert | ||
| if workspace.ID != workspaceBuild.WorkspaceID { | ||
| panic("workspace and build do not match") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Not sure if we have a panic handler, but either way this seems like it should just return an error instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others.Learn more. Ok yeah that's fair. Can you just confirm what happens when we panic in a route currently? I just want to make sure it doesn't return non-valid json and make the frontend crash or something dumb like that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Maybe we should have a middleware to catch panics and do something There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Just tied hitting a route that triggers a panic, and the server just hangs up with no response. That is, no response at all, not a 500 with an empty body or something. Not sure what the frontend will do with that... Also, the server doesn't log anything, which is unfortunate. I still think it's worthwhile panicking even without a middleware that catches them, since it will help us catch errors via unit tests of handlers that call the method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I honestly think this should be an error instead of a panic. It's an error that can be handled, not an impossible state for the program to be in (which is the only appropriate use of panic). There are multiple reasons not to panic IMO:
I definitely think we should have a panic handler for unforeseen circumstances, but we should probably never explicitly panic. At least not after the initialization code has finished. I'd also like to point to our style guide:https://github.com/golang/go/wiki/CodeReviewComments#dont-panic Just my two cents. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I don't agree that it's an error that can or should be handled. This method converts database structs representing the workspace buildand corresponding workspace to the API object. If the workspace and build don't match, that's a programming bug, which is not "normal error handling" and thus a panic is appropriate. One way to think about this is what someone should do if this condition fires. In this case, if we hit this, the problem is that the caller passed a workspace and build that don't match, and creating an object that combines them is nonsensical. The right fix will always be a code fix on the calling code, rather than handling the error at runtime. Panics in this context don't take down the server. I agree that server sending empty response isn't great, and added#1974 to track. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I see what you're saying but I don't agree, and I'm having trouble understanding what the benefit of panicing here is vs handling the error. We have a clear error handling path here, return the error and give a I also think it's a wrong motivation to panic so that function usage is cleaner (that's obviously one benefit here). And I also don't think we should be writing code for the sake of tests (i.e. can be more easily caught in a test). The end goal here is to have a robust production server, which is why I think panic is the wrong tool.
This is true, for now, but it's not far-fetched that someone would do the processing in a goroutine, at which point it would take down the server. I'm a big fan of defensive programming and returning an error here would guard against it.
👍🏻 | ||
| } | ||
| return codersdk.WorkspaceBuild{ | ||
| ID: workspaceBuild.ID, | ||
| CreatedAt: workspaceBuild.CreatedAt, | ||
| UpdatedAt: workspaceBuild.UpdatedAt, | ||
| WorkspaceID: workspaceBuild.WorkspaceID, | ||
| WorkspaceName: workspace.Name, | ||
| TemplateVersionID: workspaceBuild.TemplateVersionID, | ||
| BuildNumber: workspaceBuild.BuildNumber, | ||
| Name: workspaceBuild.Name, | ||
| Transition: codersdk.WorkspaceTransition(workspaceBuild.Transition), | ||
| InitiatorID: workspaceBuild.InitiatorID, | ||
| Job:convertProvisionerJob(job), | ||
| Deadline: workspaceBuild.Deadline, | ||
| } | ||
| } | ||