Skip to content
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

Initial Core API, and gateway integration #3207

Closed
wants to merge 2 commits into from
Closed

Initial Core API, and gateway integration #3207

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 11, 2016

Okay then, this still misses a few tests but is otherwise good for review. This uses the planned extraction of the gateway as a welcome excuse to start working on the Go side of Core API things. We're implementing the unspecified UnixfsAPI here, for now Cat() and Ls(). Near-future pull requests will implement more of UnixfsAPI (e.g. Add()), and of the Object API (i.e. SetData(), AddLink(), RmLink()).

  • core/coreapi/interface -- will eventually move to the interface-ipfs-core repo, when UnixfsAPI gets specced (out of scope right now)
  • core/coreapi -- holds the actual implementation. There's kind of a weird construct around context wiring, for the purpose of being able to pass net/http.Request.Context().

cc @kevina @Kubuxu @whyrusleeping for review.

@ghost ghost added topic/gateway Topic gateway need/review Needs a review topic/core-api Topic core-api labels Sep 11, 2016
@ghost ghost added this to the IPFS Core API milestone Sep 11, 2016
@ghost ghost added the status/in-progress In progress label Sep 11, 2016
// the hour is a hard fallback, we don't expect it to happen, but just in case
defer cancel()

if cn, ok := w.(http.CloseNotifier); ok {
Copy link
Member

Choose a reason for hiding this comment

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

why are we removing the close notify stuff?

Copy link
Author

Choose a reason for hiding this comment

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

We're wiring this to net/http.Request.Context() now (line 70)

Copy link
Member

Choose a reason for hiding this comment

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

I dont think that context implicitly has the close notify stuff on it...

Copy link
Author

Choose a reason for hiding this comment

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

Mh you're right. sloppy. I'm gonna move it into ServeHTTP() though so it also applies to POST/PUT ok?

Copy link
Author

Choose a reason for hiding this comment

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

I'm gonna move it into ServeHTTP() though so it also applies to POST/PUT ok?

... in the next PR. gonna reinstate it here for now

Copy link
Member

Choose a reason for hiding this comment

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

... in the next PR

haha, sounds good to me.

@whyrusleeping
Copy link
Member

The interfaces look pretty good to me. Simple enough to be useful all over.

@diasdavid could you take a look at it from the perspective of a 'core interface'?

@ghost
Copy link
Author

ghost commented Sep 16, 2016

Okay I updated this to improve the context wiring. The close-notify stuff is reinstated, and apiOption is gone in favor of passing contexts into the coreapi functions, in order to satisfy this rule:

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx:

@ghost ghost force-pushed the feat/coreapi branch 5 times, most recently from 0e042b5 to b7a1b66 Compare September 20, 2016 02:26
Lars Gierth added 2 commits October 28, 2016 18:00
License: MIT
Signed-off-by: Lars Gierth <[email protected]>
@ghost
Copy link
Author

ghost commented Oct 28, 2016

I'm closing this in favor of #3244 which has been based on this PR here.

@ghost ghost closed this Oct 28, 2016
@ghost ghost removed the status/in-progress In progress label Oct 28, 2016
@ghost ghost deleted the feat/coreapi branch October 28, 2016 16:27
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review topic/core-api Topic core-api topic/gateway Topic gateway
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants