IPLD In IPFS: Target Merge Branch #67
Conversation
hannahhoward
force-pushed
the
feat/ipld-in-ipfs
branch
from
8335a01 to
092b98e
Compare
hannahhoward
force-pushed
the
feat/ipld-in-ipfs
branch
from
092b98e to
a0a90a0
Compare
Add the node api as a combination of fetcher and dag writing service
Add "Node" api for prime interactions
node.go
Outdated
| type NodeAPI interface { | ||
| // fetcher.Factory provides the interface to get new dag fetchers | ||
| fetcher.Factory | ||
|
|
||
| // DagWritingService implements methods to write dags | ||
| dagwriter.DagWritingService | ||
| } |
There was a problem hiding this comment.
@warpfork do you mind taking a look on mostly if you're happy with the interfaces here being the things we expose to people who would otherwise be using the old DAG interfaces?
There was a problem hiding this comment.
Per other comment thread, are we just going to walk this back for now, or push it to future work?
I think I agree with the discussion in that thread that we just haven't put enough design cycles behind this to really commit to it being a public surface area that we'll promise to maintain, just yet.
There was a problem hiding this comment.
Yep, we're going to walk it back for now.
coreapi.go
Outdated
| @@ -22,6 +23,8 @@ type CoreAPI interface { | |||
| // Dag returns an implementation of Dag API | |||
| Dag() APIDagService | |||
|
|
|||
| Node() NodeAPI | |||
There was a problem hiding this comment.
There aren't any interface tests for the new interfaces added in this PR. @hannahhoward how much work do you think it is to add relevant tests?
I'd like to not add a new API if it's going to be untested, so if we need to push off adding the endpoint we can potentially do that.
There was a problem hiding this comment.
@aschmahmann if you are ok removing the new API, this would be my preference for the first merge. I would rather hold off on a public endpoint until we have more usage of these two modules internal to the stack (Fetcher and Dag writer). While they're tagged public modules with public methods in their own repos, that's less of a commitment than exposing them on this API -- cause when we do that, we're basically locking ourselves in. What do you think?
There was a problem hiding this comment.
If you're ok, I'll revert that part of this PR.
There was a problem hiding this comment.
yep, let's just not do this now. We don't have a story for how we want the HTTP API to handle this anyway so let's just file an issue and tackle this later.
It's a bit of a shame that it means we won't be able to tell consumers they can totally drop go-ipld-format, but that'll just have to come later.
There was a problem hiding this comment.
I'll revert that part of this PR.
IIUC that makes this PR basically dependency updates + a test fix rather than anything about IPLD + IPFS in particular. If so then maybe just make that it's own unrelated PR. If this is needed for the other work to pass or something then just leave it in this PR.
| @@ -138,7 +141,7 @@ func (tp *TestSuite) TestInvalidPathRemainder(t *testing.T) { | |||
| } | |||
|
|
|||
| _, err = api.ResolvePath(ctx, path.New("/ipld/"+nd.Cid().String()+"/bar/baz")) | |||
| if err == nil || !strings.Contains(err.Error(), "no such link found") { | |||
| if err == nil || !errors.As(err, &resolver.ErrNoLink{}) { | |||
There was a problem hiding this comment.
Seems right to me. Out of curiosity did some test pick up on this or did you just figure this was a good idea?
remove new node api until interfaces are proven
No description provided.