NotificationsYou must be signed in to change notification settings
Fork928
Star10.1k

chore: update v1 schema#643

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

Jump to bottom

Merged

coadler merged 3 commits intomainfromcolin/uuid-schema

Apr 1, 2022

Merged

chore: update v1 schema#643

coadler merged 3 commits intomainfromcolin/uuid-schema

Apr 1, 2022

Conversation

Copy link

Contributor

coadler commentedMar 29, 2022•
edited
Loading

Now uses UUIDs for users, organizations, organization members, and api keys. Also updates the v1 migration to correctly create indexes and foreign keys that were missed previously.

coadler self-assigned this

Mar 29, 2022

Copy link

codecovbot commentedMar 29, 2022•
edited
Loading

Codecov Report

Merging#643 (da23bf5) intomain (efec029) willincrease coverage by1.99%.
The diff coverage is80.21%.

@@            Coverage Diff             @@##             main     #643      +/-   ##==========================================+ Coverage   62.07%   64.06%   +1.99%==========================================  Files         113      199      +86       Lines       10755    11861    +1106       Branches        0       87      +87     ==========================================+ Hits         6676     7599     +923- Misses       3296     3439     +143- Partials      783      823      +40

Flag	Coverage Δ
unittest-go-	`63.04% <80.21%> (?)`
unittest-go-macos-latest	`59.00% <80.21%> (+0.15%)`	⬆️
unittest-go-ubuntu-latest	`61.76% <80.21%> (-0.06%)`	⬇️
unittest-go-windows-2022	`58.02% <79.67%> (?)`
unittest-js	`62.63% <ø> (?)`

Impacted Files	Coverage Δ
cli/parameters.go	`16.07% <0.00%> (-0.30%)`	⬇️
cli/workspacedelete.go	`17.64% <0.00%> (ø)`
cli/workspacelist.go	`9.52% <0.00%> (ø)`
cli/workspaces.go	`51.61% <0.00%> (ø)`
cli/workspaceshow.go	`14.28% <0.00%> (ø)`
cli/workspacestart.go	`15.15% <0.00%> (ø)`
cli/workspacestop.go	`13.88% <0.00%> (ø)`
cli/workspaceupdate.go	`7.89% <0.00%> (ø)`
codersdk/projects.go	`60.37% <ø> (ø)`
codersdk/workspacebuilds.go	`58.33% <ø> (ø)`
... and129 more

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updateefec029...da23bf5. Read thecomment docs.

coadler force-pushed thecolin/uuid-schema branch 4 times, most recently from0f51d90 tobd149b1Compare

March 30, 2022 22:31

coadler marked this pull request as ready for review

March 30, 2022 22:45

coadler requested a review fromkylecarbs

March 30, 2022 22:45

kylecarbs requested changes

Mar 30, 2022

View reviewed changes

Copy link

Member

kylecarbs left a comment

Choose a reason for hiding this comment

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

Let's trim down the initial migration as much as we possibly can. The initial schema should be 90% v2, and justsupporting v1 in a very soft way.

coderd/httpmw/httpmw.go OutdatedShow resolvedHide resolved

coderd/httpmw/organizationparam.go

		httpapi.Write(rw, http.StatusNotFound, httpapi.Response{
		Message: fmt.Sprintf("organization %q does not exist", organizationID),
		})
		orgID, ok := parseUUID(rw, r, "organization")

Copy link

Member

kylecarbsMar 30, 2022

Choose a reason for hiding this comment

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

Why abbreviate here and not fororganization? We should be consistent with abbreviations.

Copy link

ContributorAuthor

coadlerMar 31, 2022

Choose a reason for hiding this comment

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

I don't think it's necessarily important for local variables,organization is a lot to type every time. IMO it's only important for exported variables and function params.

Copy link

Member

kylecarbsApr 1, 2022

Choose a reason for hiding this comment

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

That's a bit of an arbitrarily drawn line though. It's more to type, but an inconsistent naming scheme hurts readability.

coderd/httpmw/organizationparam.go OutdatedShow resolvedHide resolved

examples/examples.go

		@@ -46,31 +47,37 @@ func List() ([]Example, error) {
		returnError = xerrors.Errorf("example %q does not contain README.md", exampleID)
		return
		}

Copy link

Member

kylecarbsMar 30, 2022

Choose a reason for hiding this comment

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

I'd rather add a linting rule to enforce newline conventions. Arbitrarily doing it on a per-developer basis will read to inconsistency when reading our code.