Show workspace name in WorkspaceBuildStats component #1933
Conversation
Signed-off-by: Spike Curtis <spike@coder.com>
| //nolint:unconvert | ||
| if workspace.ID != workspaceBuild.WorkspaceID { | ||
| panic("workspace and build do not match") |
There was a problem hiding this comment.
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.
convertWorkspaceBuild doesn't return an error, and I don't think it should. Hitting this line indicates a programming bug, not a runtime error.
There was a problem hiding this comment.
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.
Maybe we should have a middleware to catch panics and do something
There was a problem hiding this comment.
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.
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:
- Hidden danger, the caller does not know the function can "error"
- Bad user experience (server simply does not respond)
- An error takes down the whole server
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.
I don't agree that it's an error that can or should be handled. This method converts database structs representing the workspace build and 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.
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 500 response to the client. I do agree this is a programming bug, but those are very common, and that's why we perform extra checks and return errors. Especially since this is something we've anticipated can go wrong, it should not lead to a panic IMO.
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.
Panics in this context don't take down the server.
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.
I agree that server sending empty response isn't great, and added #1974 to track.
👍🏻
Signed-off-by: Spike Curtis <spike@coder.com>
* Show workspace name in WorkspaceBuildStats component Signed-off-by: Spike Curtis <spike@coder.com> * Fix WorkspaceBuildPage tests Signed-off-by: Spike Curtis <spike@coder.com>
Fixes #1801
Backend: Includes Workspace Name in returned object form WorkspaceBuild API queries --- we already get this info from the DB, so it adds negligible overhead.
Frontend: Modfies the WorkspaceBuildStats component to show the name