fix: allow either cidv{0,1} in refs-local test #2989
Conversation
|
Looking into getting this updated. Looks like I need to make the pinning conditional as the rust side doesn't have it yet.. It's a bit unobvious requirement which was probably hidden by previous use of |
| ) | ||
| // rust-ipfs doesn't yet have pinning api, it'll just list all local cids | ||
| // in /refs/local | ||
| if (common.opts.type !== 'rust') { |
There was a problem hiding this comment.
Per ipfs/js-ipfsd-ctl@master...ipfs-rust:add_rust_ipfs_http#diff-d4165b1e3527a04dd7e9d370e18c2a1aR47 (also published as draft PR to js-ipfsd-ctl) which we are using in https://github.com/ipfs-rust/ipfs-rust-conformance
|
can you please update the branch ? |
rationale: simplest way to support using cidv0 and cidv1 interchangeably in an ipfs implementation is to just convert all cids to cidv1 transparently. this has the miniscule setback of `refs/local` not being able to list cids in their original version. this fix allows the implementation to use either version internally.
| } | ||
| removed = true | ||
| delete cids[index] | ||
| break |
There was a problem hiding this comment.
Reading this again and noticed this was break was missing, added this while rebasing.
|
Test failures look transient and unrelated. |
|
Updating as we have discussed this privately with @hugomrdias and/or @achingbrain. I understand the current situation with regards to this PR is that |
jacobheun
added
status/in-progress
|
I am pretty sure this is good to close, #3124 already fixed this test case to check the multihashes instead of Cids. |
|
Confirmed! Only thing we need to patch is the pinning away. |
this is based on earlier ipfs/js-ipfs#2989 which is no longer needed.
Rationale: simplest way to support using cidv0 and cidv1 interchangeably in an ipfs implementation is to just convert all cids to cidv1 transparently. this has the miniscule setback of
refs/localnot being able to list cids in their original version. this fix allows the implementation to use either version internally.I don't know all of the lore in the cidv0 -> cidv1 migration story but the
refs/localis the only place I've seen there being an issue with doing the cidv0 -> cidv1 migration in the simplest way described above. Looking at the test I assumed the cidv0 string was expected because of "ease of test implementation" and not a specification of "any ipfs impl must be able to recover the original cid format for stored blocks inrefs/localoutput."I did think about opening up an issue of allowed cidv0 <-> cidv1 strategies but perhaps this PR is better suited for that, so please don't hesitate to let me know of the things I haven't found out yet! Could be that this approach has been tried already with bad results but I haven't found about it.
This might have a small conflict with #2980 so I'd recommend merging that first.