-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix: correctly trim resolved IPNS addresses #7331
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.
LGTM, nice typo catch.
Two things about the test that was added though.
- These tests (fuse) don't run in CI which means we should probably be pretty careful around especially around releases to run these tests locally (and ideally have some way of testing FUSE in CI, but that's a separate issue)
- The test covers this bug in particular, but it leaves the code path of resolving a key published by another node (or a key published by us other than the
self
key) untested. There's no reason to block the PR on this, but we should file an issue to track it.
Edit: @Stebalien I ran the new test manually just to make sure it works and it fails on my machine with No such file or directory
. Does it work successfully for you?
This test is tests resolving keys through the We do need to start running this in CI, but we'll need a special job for fuse-only test.
It does... do the other tests work? |
@@ -53,6 +55,12 @@ test_expect_success FUSE "'ipfs mount' output looks good" ' | |||
test_cmp expected actual | |||
' | |||
|
|||
test_expect_success FUSE "can resolve ipns names" ' |
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.
Continuing from #7331 (comment) so we can do this in a thread-like fashion.
Running ./t0030-mount.sh -v
passes on master, but with this branch fails with:
cat: ipns/welcome.example.com/ping: No such file or directory
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.
Did you make sure to rebuild? Otherwise, I have no idea. Does ipns/local
exist when running the test?
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.
I think I had a rebuilding issue... so dumb. Sorry for the confusion
fixes #7246