Skip to content

feat: update build url to @username/workspace/builds/buildnumber #2234

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 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 5 commits into from
Jun 10, 2022

Conversation

AbhineetJain
Copy link
Contributor

This PR updates the build URL from /builds/<build_id> to /@/<workspace_name>/builds/<build_number>.

Subtasks

  • add new query to get build via build number
  • add new API endpoint
  • update builds route
  • update API call from workspace build machine
  • update the machine to call logs after getting build
  • update tests

Fixes #1906

Screenshot

Screen Shot 2022-06-10 at 1 49 01 AM

@AbhineetJain AbhineetJain requested a review from a team as a code owner June 10, 2022 05:49
@AbhineetJain AbhineetJain requested a review from a team June 10, 2022 05:50
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Here's some feedback from the backend perspective:

  • We should add some unit tests whenever adding a new API route
  • HTTP errors related to client request parameters are generally 4XX.
  • Don't forget to handle sql.ErrNoRows!

Comment on lines +214 to +233
users, err := api.Database.GetUsersByIDs(r.Context(), []uuid.UUID{workspace.OwnerID, workspaceBuild.InitiatorID})
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: "Internal error fetching user.",
Detail: err.Error(),
})
return
}
Copy link
Member

Choose a reason for hiding this comment

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

Remark (non-blocking): If we end up having the concept of "system" or "ghost" users with reserved UUIDs, we'll need to make sure that this findUser function is aware of these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think it worth making a pass through the repo to find such occurrences and commenting around them for future reference?

Copy link
Member

Choose a reason for hiding this comment

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

Do you think it worth making a pass through the repo to find such occurrences and commenting around them for future reference?

Not just yet. It's more of something to keep in mind if or when we do have "system" or "ghost" users.

Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

Nice PR with both front- and backend changes! Cian already left a great (backend) review so not much to add.

@AbhineetJain AbhineetJain force-pushed the abhineetjain/1906-build-url-change branch from b2b5fe5 to e60aa38 Compare June 10, 2022 15:16
@AbhineetJain AbhineetJain requested a review from johnstcn June 10, 2022 15:17
Copy link
Member

@johnstcn johnstcn left a 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 from the backend perspective!
I'll defer to others on the frontend part.

export const WorkspaceBuildPage: FC = () => {
const buildId = useBuildId()
const [buildState] = useMachine(workspaceBuildMachine, { context: { buildId } })
const { username, workspace: workspaceName, buildNumber } = useParams()
Copy link
Member

Choose a reason for hiding this comment

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

ya, just like this! and I love that alias 🎈

@kylecarbs
Copy link
Member

I won't have time to review it today, but the others got it! 🥳🥳🥳

Copy link
Member

@Kira-Pilot Kira-Pilot left a comment

Choose a reason for hiding this comment

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

FE ✔️

@AbhineetJain AbhineetJain merged commit b2833c6 into main Jun 10, 2022
@AbhineetJain AbhineetJain deleted the abhineetjain/1906-build-url-change branch June 10, 2022 16:08
kylecarbs pushed a commit that referenced this pull request Jun 10, 2022
* update build url to @username/workspace/builds/buildnumber

* update errors thrown from the API

* add unit tests for the new API

* add t.parallel

* get username and workspace name from params
Kira-Pilot added a commit that referenced this pull request Jun 14, 2022
* feat: update build url to @username/workspace/builds/buildnumber (#2234)

* update build url to @username/workspace/builds/buildnumber

* update errors thrown from the API

* add unit tests for the new API

* add t.parallel

* get username and workspace name from params

* fix: update icon (#2216)

* feat: Show template description in `coder template init` (#2238)

* fix: workspace schedule time displays (#2249)

Summary:

Various time displays weren't quite right.

Details:

- Display date (not just time) of upcoming workspace stop in workspace
page
- Fix ttlShutdownAt for various cases + tests
  - manual to non-manual
  - unchanged/unmodified
  - isBefore --> isSameOrBefore
  - use the delta (off by _ error)
- pluralize units in dayjs.add

* fix: Remove easter egg mentioning competitor (#2250)

This is more confusing than helpful!

* feat: Warn on coderd startup if access URL is localhost (#2248)

* feat: use custom wireguard reverse proxy for dev tunnel (#1975)

* fix: use correct link in create from template button (#2253)

* feat: store and display template creator (#2228)

* design commit

* add owner_id to templates table

* add owner information in apis and ui

* update minWidth for statItem

* rename owner to created_by

* missing refactor to created_by

* handle errors in fetching created_by names

* feat: update language on workspace page (#2220)

* fix: ensure config dir exists before reading tunnel config (#2259)

* fix(devtunnel): close `http.Server` before wireguard interface (#2263)

* fix: ensure `agentResource` is non-nil (#2261)

* chore: add hero image to OSS docs homepage (#2241)

* fix: Do not write 2 errors to api on template fetch error (#2285)

* feat: add tooltips to templates page

resolves #2242

Co-authored-by: Abhineet Jain <AbhineetJain@users.noreply.github.com>
Co-authored-by: Joe Previte <jjprevite@gmail.com>
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
Co-authored-by: G r e y <grey@coder.com>
Co-authored-by: Kyle Carberry <kyle@coder.com>
Co-authored-by: David Wahler <david@coder.com>
Co-authored-by: Colin Adler <colin1adler@gmail.com>
Co-authored-by: Garrett Delfosse <garrett@coder.com>
Co-authored-by: Katie Horne <katie@coder.com>
Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show "build log number" in UI/CLI
5 participants