Skip to content

Add endpoint documentation#6290

Merged
chiiph merged 5 commits intomainfrom
endpoint-documentation
Jun 22, 2022
Merged

Add endpoint documentation#6290
chiiph merged 5 commits intomainfrom
endpoint-documentation

Conversation

@chiiph
Copy link
Copy Markdown
Contributor

@chiiph chiiph commented Jun 20, 2022

No description provided.

@chiiph chiiph requested review from a team and zwass June 20, 2022 21:11
Copy link
Copy Markdown
Member

@zwass zwass left a comment

Choose a reason for hiding this comment

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

Nice summary! A few copy edits from me.

Would be great to make sure that someone who's been working with this more recently gets eyes on this before merging.

Comment thread docs/Contributing/Adding-new-endpoints.md Outdated
Comment thread docs/Contributing/Adding-new-endpoints.md Outdated
Comment thread docs/Contributing/Adding-new-endpoints.md Outdated
roperzh
roperzh previously approved these changes Jun 21, 2022
Copy link
Copy Markdown
Contributor

@roperzh roperzh left a comment

Choose a reason for hiding this comment

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

❤️ very very nice, and super useful.

It would be amazing if we also mention some of the logic we have in place to modify the response (which can't be done directly at the handler):

if render, ok := response.(renderHijacker); ok {
render.hijackRender(ctx, w)
return nil
}
if e, ok := response.(statuser); ok {
w.WriteHeader(e.Status())
if e.Status() == http.StatusNoContent {
return nil
}
}

Comment thread docs/Contributing/Adding-new-endpoints.md Outdated
Comment on lines +50 to +52
In order to use this new Datastore function we created, the layer that is in communication with it is the `Service`
which is both [an interface](https://github.com/fleetdm/fleet/blob/main/server/fleet/service.go#L41) and
[a struct](https://github.com/fleetdm/fleet/blob/main/server/service/service.go#L25) that implements that interface.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it would benefit the reader if we're more precise here, we have an interface fleet.Service and an struct service.Service that implements the interface.

Comment thread docs/Contributing/Adding-new-endpoints.md Outdated
Comment thread docs/Contributing/Adding-new-endpoints.md Outdated
Copy link
Copy Markdown
Member

@mna mna left a comment

Choose a reason for hiding this comment

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

LGTM! Left a couple comments about possible missing pieces of info.

Comment thread docs/Contributing/Adding-new-endpoints.md
Comment thread docs/Contributing/Adding-new-endpoints.md Outdated
@chiiph chiiph temporarily deployed to Docker Hub June 21, 2022 14:12 Inactive
@chiiph chiiph temporarily deployed to Docker Hub June 21, 2022 14:28 Inactive
roperzh
roperzh previously approved these changes Jun 21, 2022
mna
mna previously approved these changes Jun 21, 2022
Comment thread docs/Contributing/Adding-new-endpoints.md Outdated
Comment thread docs/Contributing/Adding-new-endpoints.md Outdated
lucasmrod
lucasmrod previously approved these changes Jun 21, 2022
@chiiph chiiph dismissed stale reviews from lucasmrod, mna, and roperzh via ec9284b June 21, 2022 19:08
@chiiph chiiph temporarily deployed to Docker Hub June 21, 2022 19:08 Inactive
@chiiph chiiph merged commit 1faa3c4 into main Jun 22, 2022
@chiiph chiiph deleted the endpoint-documentation branch June 22, 2022 14:34
Desmi-Dizney added a commit that referenced this pull request Jun 28, 2022
@Desmi-Dizney
Copy link
Copy Markdown
Contributor

Editor pass completed on:

Desmi-Dizney added a commit that referenced this pull request Jun 29, 2022
DominusKelvin pushed a commit that referenced this pull request Jul 5, 2022
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.

6 participants