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

refactor: replace Code by Detail in the http API error#1011

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

BrunoQuaresma merged 5 commits intomainfrombq/1008/replace-code-by-detail

Apr 18, 2022

Merged

refactor: replace Code by Detail in the http API error#1011

BrunoQuaresma merged 5 commits intomainfrombq/1008/replace-code-by-detail

Apr 18, 2022

Conversation

Copy link

Collaborator

BrunoQuaresma commentedApr 14, 2022

Closes#1008

refactor: replace Code by Detail in the http API error

49f0ffe

BrunoQuaresma requested review fromkylecarbs andgreyscaled

April 14, 2022 19:14

BrunoQuaresma self-assigned this

Apr 14, 2022

Copy link

CollaboratorAuthor

BrunoQuaresma commentedApr 14, 2022

Should we create a function to centralize these messages?

fix: http api test

55bf028

Copy link

codecovbot commentedApr 14, 2022•
edited
Loading

Codecov Report

Merging#1011 (35f9f18) intomain (76f8ff9) willincrease coverage by0.02%.
The diff coverage is93.33%.

@@            Coverage Diff             @@##             main    #1011      +/-   ##==========================================+ Coverage   67.08%   67.10%   +0.02%==========================================  Files         256      256                Lines       14986    14986                Branches      136      136              ==========================================+ Hits        10053    10056       +3+ Misses       3938     3935       -3  Partials      995      995

Flag	Coverage Δ
unittest-go-macos-latest	`53.57% <93.33%> (-0.14%)`	⬇️
unittest-go-postgres-	`66.76% <93.33%> (+0.05%)`	⬆️
unittest-go-ubuntu-latest	`56.12% <93.33%> (-0.21%)`	⬇️
unittest-go-windows-2022	`53.03% <93.33%> (+0.02%)`	⬆️
unittest-js	`62.94% <ø> (ø)`

Impacted Files	Coverage Δ
codersdk/client.go	`59.74% <0.00%> (ø)`
coderd/httpapi/httpapi.go	`72.50% <100.00%> (ø)`
coderd/organizations.go	`60.64% <100.00%> (ø)`
coderd/users.go	`59.60% <100.00%> (ø)`
coderd/workspaces.go	`55.62% <100.00%> (ø)`
coderd/parameter/compute.go	`74.07% <0.00%> (-6.67%)`	⬇️
provisionerd/provisionerd.go	`82.35% <0.00%> (-0.45%)`	⬇️
coderd/database/queries.sql.go	`83.65% <0.00%> (-0.42%)`	⬇️
coderd/provisionerdaemons.go	`63.97% <0.00%> (+0.51%)`	⬆️
peer/conn.go	`82.23% <0.00%> (+1.77%)`	⬆️
... and3 more

Continue to review full report at Codecov.

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

greyscaled reviewed

Apr 14, 2022

View reviewed changes

coderd/users.go OutdatedShow resolvedHide resolved

greyscaled reviewed

Apr 14, 2022

View reviewed changes

coderd/users.goShow resolvedHide resolved

BrunoQuaresmaand others added3 commits

April 14, 2022 16:35

Update coderd/users.go

c4162df

Co-authored-by: G r e y <grey@coder.com>

fix: replace Code by Detail

5015f1c

chore: merge branch 'bq/1008/replace-code-by-detail' of github.com:co…

35f9f18

…der/coder into bq/1008/replace-code-by-detail

BrunoQuaresma requested a review fromgreyscaled

April 15, 2022 14:15

greyscaled approved these changes

Apr 15, 2022

View reviewed changes

Copy link

Contributor

greyscaled left a comment

Choose a reason for hiding this comment

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

This looks good to me, but we should get approval from @coder/backend

greyscaled requested a review froma team

April 15, 2022 19:01

johnstcn reviewed

Apr 15, 2022

View reviewed changes

Copy link

Member

johnstcn left a comment

Choose a reason for hiding this comment

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

As far as I know, the idea of theCode field is to allow a machine-readable error message.
Should we maybe keep bothCode andDetail fields instead?

Copy link

Member

kylecarbs commentedApr 18, 2022

The idea short-term is to try eliminating machine-readable messages in favor of HTTP response codes. If that doesn't work, we'll probably have to do something like that.