-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: make "ipfs resolve" cli command recursive by default #3707
fix: make "ipfs resolve" cli command recursive by default #3707
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! CI is green, but it also means that we have no tests for this 😅
Perhaps you could add DNSLink test to safeguard of the default behavior in ipfs-cli/test/resolve.js?
I updated the tests a bit in @lidel Do you know if there's an end-to-end test for the resolve method elsewhere? |
@yusefnapora the intergration-style tests that would actually do DNS lookups are in the interface-ipfs-core tests - please add new tests for this there. |
@achingbrain thanks for the pointer! There was already a test in there for the recursive case, but since recursive is the default, there was no test that covered the non-recursive case. I added a test for So if e.g. you have a recursive IPNS record like:
calling Yay for tests :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgm, tests seem to fail for unrelated reasons, @achingbrain mind confirming and doing final review in spare time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, but the newly added test is failing in CI
Still failing:
|
@yusefnapora will you have time to push this one over the finish line? |
Sorry for letting this one dangle folks. I'll pick it back up this week |
This fixes an issue where a non-recursive lookup would change a result like `/ipns/foo` into `/ipfs/foo`.
The last few commits were me flailing to figure out a type check failure in The problem is that on node, SO to the rescue with a trick to define the type of |
BTW, I think the original test failure was happening because I used the same key ID ( |
There's one more test failure, but this one is a module resolution failure in example code:
That sounds like something that fits into the "maintenance week" remit, so I'll check it out tomorrow :) That should probably go into its own PR though. |
@achingbrain it looks like the last failing test should be fixed by the work you did on #3742 😄 |
@yusefnapora awesome! I've restarted that bit of the build, it should pull down the updated modules and be good then. |
I'm going to land this after #3556 |
This changes the default behavior of the
jsipfs resolve
cli command to be recursive by default. Closes #3692.# in packages/ipfs node src/cli.js resolve /ipns/ipfs.io /ipfs/bafybeiagozluzfopjadeigrjlsmktseozde2xc5prvighob7452imnk76a
BREAKING CHANGE: resolve is now recursive by default