Skip to content

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 1, 2022
Merged

Show workspace name in WorkspaceBuildStats component #1933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa37450
Show workspace name in WorkspaceBuildStats component
spikecurtis May 31, 2022
2f3f3f8
Fix WorkspaceBuildPage tests
spikecurtis May 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 15 additions & 13 deletions coderd/workspacebuilds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,7 @@ import (

func (api *API) workspaceBuild(rw http.ResponseWriter, r *http.Request) {
workspaceBuild := httpmw.WorkspaceBuildParam(r)
workspace, err := api.Database.GetWorkspaceByID(r.Context(), workspaceBuild.WorkspaceID)
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: "no workspace exists for this job",
})
return
}
workspace := httpmw.WorkspaceParam(r)

if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceWorkspace.
InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) {
Expand All @@ -42,7 +36,7 @@ func (api *API) workspaceBuild(rw http.ResponseWriter, r *http.Request) {
return
}

httpapi.Write(rw, http.StatusOK, convertWorkspaceBuild(workspaceBuild, convertProvisionerJob(job)))
httpapi.Write(rw, http.StatusOK, convertWorkspaceBuild(workspace, workspaceBuild, job))
}

func (api *API) workspaceBuilds(rw http.ResponseWriter, r *http.Request) {
Expand Down Expand Up @@ -101,7 +95,7 @@ func (api *API) workspaceBuilds(rw http.ResponseWriter, r *http.Request) {
})
return
}
apiBuilds = append(apiBuilds, convertWorkspaceBuild(build, convertProvisionerJob(job)))
apiBuilds = append(apiBuilds, convertWorkspaceBuild(workspace, build, job))
}

httpapi.Write(rw, http.StatusOK, apiBuilds)
Expand Down Expand Up @@ -139,7 +133,7 @@ func (api *API) workspaceBuildByName(rw http.ResponseWriter, r *http.Request) {
return
}

httpapi.Write(rw, http.StatusOK, convertWorkspaceBuild(workspaceBuild, convertProvisionerJob(job)))
httpapi.Write(rw, http.StatusOK, convertWorkspaceBuild(workspace, workspaceBuild, job))
}

func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) {
Expand Down Expand Up @@ -307,7 +301,8 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) {
return
}

httpapi.Write(rw, http.StatusCreated, convertWorkspaceBuild(workspaceBuild, convertProvisionerJob(provisionerJob)))
httpapi.Write(rw, http.StatusCreated,
convertWorkspaceBuild(workspace, workspaceBuild, provisionerJob))
}

func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Request) {
Expand Down Expand Up @@ -432,19 +427,26 @@ func (api *API) workspaceBuildState(rw http.ResponseWriter, r *http.Request) {
_, _ = rw.Write(workspaceBuild.ProvisionerState)
}

func convertWorkspaceBuild(workspaceBuild database.WorkspaceBuild, job codersdk.ProvisionerJob) codersdk.WorkspaceBuild {
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")
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

convertWorkspaceBuild doesn't return an error, and I don't think it should. Hitting this line indicates a programming bug, not a runtime error.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should have a middleware to catch panics and do something

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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:

  • 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 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.

👍🏻

}
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: job,
Job: convertProvisionerJob(job),
Deadline: workspaceBuild.Deadline,
}
}
Expand Down
23 changes: 12 additions & 11 deletions coderd/workspaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,7 @@ func (api *API) workspace(rw http.ResponseWriter, r *http.Request) {
return
}

httpapi.Write(rw, http.StatusOK,
convertWorkspace(workspace, convertWorkspaceBuild(build, convertProvisionerJob(job)), template, owner))
httpapi.Write(rw, http.StatusOK, convertWorkspace(workspace, build, job, template, owner))
}

func (api *API) workspacesByOrganization(rw http.ResponseWriter, r *http.Request) {
Expand Down Expand Up @@ -275,8 +274,7 @@ func (api *API) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request)
return
}

httpapi.Write(rw, http.StatusOK, convertWorkspace(workspace,
convertWorkspaceBuild(build, convertProvisionerJob(job)), template, owner))
httpapi.Write(rw, http.StatusOK, convertWorkspace(workspace, build, job, template, owner))
}

// Create a new workspace for the currently authenticated user.
Expand Down Expand Up @@ -514,8 +512,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
return
}

httpapi.Write(rw, http.StatusCreated, convertWorkspace(workspace,
convertWorkspaceBuild(workspaceBuild, convertProvisionerJob(templateVersionJob)), template, user))
httpapi.Write(rw, http.StatusCreated, convertWorkspace(workspace, workspaceBuild, templateVersionJob, template, user))
}

func (api *API) putWorkspaceAutostart(rw http.ResponseWriter, r *http.Request) {
Expand Down Expand Up @@ -736,7 +733,7 @@ func (api *API) watchWorkspace(rw http.ResponseWriter, r *http.Request) {
return
}

_ = wsjson.Write(ctx, c, convertWorkspace(workspace, convertWorkspaceBuild(build, convertProvisionerJob(job)), template, owner))
_ = wsjson.Write(ctx, c, convertWorkspace(workspace, build, job, template, owner))
case <-ctx.Done():
return
}
Expand Down Expand Up @@ -829,21 +826,25 @@ func convertWorkspaces(ctx context.Context, db database.Store, workspaces []data
if !exists {
return nil, xerrors.Errorf("owner not found for workspace: %q", workspace.Name)
}
apiWorkspaces = append(apiWorkspaces,
convertWorkspace(workspace, convertWorkspaceBuild(build, convertProvisionerJob(job)), template, user))
apiWorkspaces = append(apiWorkspaces, convertWorkspace(workspace, build, job, template, user))
}
return apiWorkspaces, nil
}

func convertWorkspace(workspace database.Workspace, workspaceBuild codersdk.WorkspaceBuild, template database.Template, owner database.User) codersdk.Workspace {
func convertWorkspace(
workspace database.Workspace,
workspaceBuild database.WorkspaceBuild,
job database.ProvisionerJob,
template database.Template,
owner database.User) codersdk.Workspace {
return codersdk.Workspace{
ID: workspace.ID,
CreatedAt: workspace.CreatedAt,
UpdatedAt: workspace.UpdatedAt,
OwnerID: workspace.OwnerID,
OwnerName: owner.Username,
TemplateID: workspace.TemplateID,
LatestBuild: workspaceBuild,
LatestBuild: convertWorkspaceBuild(workspace, workspaceBuild, job),
TemplateName: template.Name,
Outdated: workspaceBuild.TemplateVersionID.String() != template.ActiveVersionID.String(),
Name: workspace.Name,
Expand Down
1 change: 1 addition & 0 deletions codersdk/workspacebuilds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ type WorkspaceBuild struct {
CreatedAt time.Time `json:"created_at"`
UpdatedAt time.Time `json:"updated_at"`
WorkspaceID uuid.UUID `json:"workspace_id"`
WorkspaceName string `json:"workspace_name"`
TemplateVersionID uuid.UUID `json:"template_version_id"`
BuildNumber int32 `json:"build_number"`
Name string `json:"name"`
Expand Down
1 change: 1 addition & 0 deletions site/src/api/typesGenerated.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,7 @@ export interface WorkspaceBuild {
readonly created_at: string
readonly updated_at: string
readonly workspace_id: string
readonly workspace_name: string
readonly template_version_id: string
readonly build_number: number
readonly name: string
Expand Down
4 changes: 2 additions & 2 deletions site/src/components/WorkspaceBuildStats/WorkspaceBuildStats.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ export const WorkspaceBuildStats: FC<WorkspaceBuildStatsProps> = ({ build }) =>
return (
<div className={styles.stats}>
<div className={styles.statItem}>
<span className={styles.statsLabel}>Workspace ID</span>
<span className={styles.statsLabel}>Workspace Name</span>
<Link
component={RouterLink}
to={`/workspaces/${build.workspace_id}`}
className={combineClasses([styles.statsValue, styles.link])}
>
{build.workspace_id}
{build.workspace_name}
</Link>
</div>
<div className={styles.statsDivider} />
Expand Down
2 changes: 1 addition & 1 deletion site/src/pages/WorkspaceBuildPage/WorkspaceBuildPage.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ describe("WorkspaceBuildPage", () => {
it("renders the stats and logs", async () => {
renderWithAuth(<WorkspaceBuildPage />, { route: `/builds/${MockWorkspaceBuild.id}`, path: "/builds/:buildId" })

await screen.findByText(MockWorkspaceBuild.workspace_id)
await screen.findByText(MockWorkspaceBuild.workspace_name)
await screen.findByText(MockWorkspaceBuildLogs[0].stage)
})
})
3 changes: 2 additions & 1 deletion site/src/testHelpers/entities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,8 @@ export const MockWorkspaceBuild: TypesGen.WorkspaceBuild = {
template_version_id: "",
transition: "start",
updated_at: "2022-05-17T17:39:01.382927298Z",
workspace_id: "test-workspace",
workspace_name: "test-workspace",
workspace_id: "759f1d46-3174-453d-aa60-980a9c1442f3",
deadline: "2022-05-17T23:39:00.00Z",
}

Expand Down