update the codec names for the dag put command: ipld-in-ipfs series of PRs #248
Conversation
This comment has been minimized.
This comment has been minimized.
shell_test.go
Outdated
| @@ -398,7 +398,7 @@ func TestDagPutWithOpts(t *testing.T) { | |||
|
|
|||
| c, err := s.DagPutWithOpts(`{"x": "abc","y":"def"}`, options.Dag.Pin("true")) | |||
| is.Nil(err) | |||
| is.Equal(c, "bafyreidrm3r2k6vlxqp2fk47sboeycf7apddib47w7cyagrajtpaxxl2pi") | |||
| is.Equal(c, "bafireidrm3r2k6vlxqp2fk47sboeycf7apddib47w7cyagrajtpaxxl2pi") | |||
There was a problem hiding this comment.
Why are these CIDs different?
There was a problem hiding this comment.
I believe this is the codec, which we now account for correctly as dag-json rather than json
There was a problem hiding this comment.
https://cid.ipfs.io/#bafireidrm3r2k6vlxqp2fk47sboeycf7apddib47w7cyagrajtpaxxl2pi
https://cid.ipfs.io/#bafyreidrm3r2k6vlxqp2fk47sboeycf7apddib47w7cyagrajtpaxxl2pi
Are you sure, it looks like it's because you changed the output encoding from cbor to dag-cbor. This shouldn't be needed, right? Unless the idea is that we're testing the defaults here.
There was a problem hiding this comment.
sorry, i knew it was adding the dag.
The default is now dag-cbor for the encoding. (in that we don't fail if you try to put in a cid link in your dag put)
There was a problem hiding this comment.
IIUC what's happening here is that we used to say cbor = dagcbor and the default is cbor. We now have cbor as a separate option from dag-cbor, but the default has changed to dag-cbor.
I'd think that if you revert this change, but just temporarily bump CI to point the go-ipfs target branch that things will pass.
There was a problem hiding this comment.
note: reading this again, i now believe this shouldn't pass without this change: line 424 of the current master of this repo explicitly overrides the default back to cbor.
|
To verify that CI passes you may want to temporarily add a commit This circular dependency is annoying, but I'm not sure we have good alternatives at the moment. |
It's not a commit. the CI is running against a docker image of ipfs. It seems non-trivial to change the IPFS this runs against? |
That does seem like a bit of a pain. We do build docker containers in CI so reaching out to see how easy it is to reference them in CI here. Probably we should ditch docker here and just download and run the go binary like we do in the other http client https://github.com/ipfs/go-ipfs-http-client, but I'd like to minimize the effort that goes into dealing with this particular set of PRs. |
|
The tests are broken, but only because the version of go-ipfs CI is testing against is out of date. Tested to pass in #249 and we'll rerun CI once ipfs/kubo#7976 is merged. |
aschmahmann
changed the title
|
CI on go-ipfs master is currently broken. |
|
@Stebalien if you want to merge the changes from #249 they'll pass again. Otherwise, we're waiting until the IPLD in IPFS PR is merged |
No description provided.