Skip to content

daemon: JSON responses for API#546

Merged
yaoharry merged 24 commits into
masterfrom
daemon/#502-json-responses
Feb 12, 2019
Merged

daemon: JSON responses for API#546
yaoharry merged 24 commits into
masterfrom
daemon/#502-json-responses

Conversation

@bobheadxi

@bobheadxi bobheadxi commented Feb 10, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

🎟️ Ticket(s): Closes #502

👷 Changes

  • chi + middleware for routing needs
  • basic JSON structure for as many things as I could update without demolishing things (package api and package res)
  • update CLI and stuff as much as possible without radical changes

Here's the idea - all responses should be structured like this:

// BaseResponse is the underlying response structure to all responses.
type BaseResponse struct {
	// Basic metadata
	HTTPStatusCode int    `json:"code"`
	RequestID      string `json:"request_id,omitempty"`

	// Message is included in all responses, and is a summary of the server's response
	Message string `json:"message"`

	// Error contains additional context in the event of an error
	Error string `json:"error,omitempty"`

	// Data contains information the server wants to return
	Data interface{} `json:"data,omitempty"`
}

Data is a json object of some sort. For example, login response looks like:

{
   "code":200,
   "request_id":"bobbook/2Mch7LMzhj-000001",
   "message":"session created",
   "data":{
      "token":"eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzZXNzaW9uX2lkIjoiTEZ4cGVsMTc3R1hMd3d6Y281UjhNU0Z5eDZadWFYa0FQcFJpa25iRzhZTT0iLCJ1c2VyIjoiYm9iaGVhZHhpIiwiYWRtaW4iOmZhbHNlLCJleHBpcnkiOiIyMDE5LTAyLTEyVDA1OjAxOjM2LjgzMTQwNi0wODowMCJ9.0cszidOwITpIU3ZidENTwfnFBdorkkApXXfUkfUDsi0"
   }
}

and totp/enable looks like:

{
   "code":200,
   "request_id":"bobbook/4NVeCCyrcK-000002",
   "message":"TOTP successfully enabled",
   "data":{
      "totp":{
         "secret":"D3YPABNVY6GBK3EH",
         "backup_codes":[
            "4aac6-44509",
            "a3415-2ccec",
            "5c14b-cbdb5",
            "3376e-0ca9c",
            "487ba-722f7",
            "63f2c-040c1",
            "9b0d6-4b561",
            "64442-157de",
            "497ee-07888",
            "be823-a7aa4"
         ]
      }
   }
}

there's also a handy new utility for working with these responses clientside (not used much yet, but will leave for another PR):

var totpResp = &api.TotpResponse{}
api.Unmarshal(resp.Body, api.KV{Key: "totp", Value: totpResp})
fmt.Printf("%+v", totpResp) // { "secret": "...", "backup_codes": [ "...", ... ] }

This PR is gonna help out #109, #236, #389 and probably more

🔦 Testing Instructions

run tests

also run testenv and try out all the inertia [remote] commands, they'll look ugly but... they shouldnt break (I hope 😋 )

@bobheadxi bobheadxi added the pr: finalized needs review and final approval label Feb 10, 2019
@bobheadxi

bobheadxi commented Feb 10, 2019

Copy link
Copy Markdown
Member Author

@mRabitsky would appreciate your thoughts on this one!

@codecov

codecov Bot commented Feb 11, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #546 into master will increase coverage by 0.17%.
The diff coverage is 49.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #546      +/-   ##
==========================================
+ Coverage   56.21%   56.37%   +0.17%     
==========================================
  Files          60       61       +1     
  Lines        2991     3000       +9     
==========================================
+ Hits         1681     1691      +10     
- Misses       1102     1104       +2     
+ Partials      208      205       -3
Impacted Files Coverage Δ
daemon/inertiad/daemon/prune.go 0% <0%> (ø) ⬆️
daemon/inertiad/daemon/env.go 0% <0%> (ø) ⬆️
daemon/inertiad/daemon/up.go 0% <0%> (ø) ⬆️
daemon/inertiad/daemon/reset.go 0% <0%> (ø) ⬆️
daemon/inertiad/daemon/logs.go 0% <0%> (ø) ⬆️
daemon/inertiad/auth/sessions.go 82.06% <100%> (ø) ⬆️
daemon/inertiad/daemon/webhook.go 26.23% <12.5%> (-0.43%) ⬇️
daemon/inertiad/daemon/down.go 23.53% <12.5%> (-1.47%) ⬇️
daemon/inertiad/daemon/token.go 45.46% <50%> (-4.54%) ⬇️
daemon/inertiad/auth/users.go 73.6% <50%> (-0.64%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84b5b77...de51c41. Read the comment docs.

@bobheadxi bobheadxi mentioned this pull request Feb 11, 2019
Merged
@yaoharry yaoharry merged commit ce57052 into master Feb 12, 2019
@bobheadxi bobheadxi deleted the daemon/#502-json-responses branch February 12, 2019 06:54
bobheadxi added a commit that referenced this pull request Feb 12, 2019
@bobheadxi
daemon: JSON responses for API (#546)
3806f4b 
* chi (https://github.com/go-chi/chi) + chi middleware for routing needs

* basic JSON structure for as many things as I could update without demolishing things (package `api` and package `res`)

* update CLI and client as much as possible without radical changes for new API responses

* minor refactors to package log (rename `Logger` to `Streamer`, update success and error funcs to take render.Renderer)
mRabitsky
mRabitsky reviewed Feb 12, 2019
View reviewed changes

@mRabitsky mRabitsky left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Current state of the API looks good. Future considerations for the API may include standardizing it using either the OAS or RAML to declaritively specify our API and make it easier to work with considering the language barrier existing between the front/back-end.

@bobheadxi

bobheadxi commented Feb 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

@mRabitsky by OAS, do you mean Swagger?

update: opened #556 to address this!

@bobheadxi bobheadxi mentioned this pull request Feb 13, 2019
Closed
@bobheadxi bobheadxi mentioned this pull request Feb 24, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yaoharry yaoharry yaoharry approved these changes

@kimoantiqe kimoantiqe Awaiting requested review from kimoantiqe

@nicholaschinjie nicholaschinjie Awaiting requested review from nicholaschinjie

@seifghazi seifghazi Awaiting requested review from seifghazi

@terryz21 terryz21 Awaiting requested review from terryz21

+1 more reviewer

@mRabitsky mRabitsky mRabitsky left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr: finalized needs review and final approval

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bobheadxi @mRabitsky @yaoharry