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

object-patch: Support linking to non-dagpb objects #4460

Merged
merged 3 commits into from
Dec 7, 2017

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented Dec 6, 2017

No description provided.

License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@ghost ghost assigned magik6k Dec 6, 2017
@ghost ghost added the status/in-progress In progress label Dec 6, 2017
@magik6k
Copy link
Member Author

magik6k commented Dec 6, 2017

Random fails on jenkins:

not ok 5 - request looks good
#	
#	  grep "POST /api/v0/cat" nc_out
#	

expecting success: 
  test_expect_code 1 grep "api=/ip4" nc_out

Got this one 3 times:

expecting success: 
      ipfsi "$node" name resolve "$name" > resolve
    
Error: Could not resolve name.
not ok 20 - just after publishing: node 3 can resolve entry
#	
#	      ipfsi "$node" name resolve "$name" > resolve
#	

@Stebalien
Copy link
Member

not ok 20 - just after publishing: node 3 can resolve entry

Yeah, I've seen that a few times recently. @vyzo?

@vyzo
Copy link
Contributor

vyzo commented Dec 7, 2017

think it's related to the new pubsub code and changes that it brought? It really shouldn't affect normal resolutions.

@@ -212,8 +212,9 @@ func copyDag(nd *dag.ProtoNode, from, to dag.DAGService) error {
}

childpb, ok := child.(*dag.ProtoNode)
if !ok {
return dag.ErrNotProtobuf
if !ok { // leaf node
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a bit risky to do this way. Lets switch on the type, and then explicitly handle the ProtoNode and RawNodes. Then for everything else we can error out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not allow linking to other types? I actually need this to link to git commits, it works well for my usecase (ipld git helper). (note that it's already possible to do this with dag commands)

Copy link
Member

Choose a reason for hiding this comment

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

The issue isnt that we don't want to allow linking, but that this operation is supposed to recursively copy a graph from one dagservice to another.

Copy link
Member

Choose a reason for hiding this comment

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

@magik6k we can change the first argument of this function to accept a node.Node, then skip this check entirely

Copy link
Member Author

Choose a reason for hiding this comment

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

done

EMPTY_DIR=$(ipfs object new unixfs-dir) &&
OBJ=$(echo "123" | ipfs dag put) &&
N1=$(ipfs object patch $EMPTY_DIR add-link foo $OBJ) &&
ipfs object stat $N1 > /dev/null
Copy link

Choose a reason for hiding this comment

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

This should also test resolving the link, i.e. $N1/foo

License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@whyrusleeping whyrusleeping merged commit a4f9333 into master Dec 7, 2017
@whyrusleeping whyrusleeping deleted the fix/object-patch-nonpb branch December 7, 2017 23:10
@ghost ghost removed the status/in-progress In progress label Dec 7, 2017
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.

4 participants