Skip to content
This repository was archived by the owner on Aug 1, 2023. It is now read-only.

fix: all repo compatibility tests#109

Merged
achingbrain merged 1 commit into
masterfrom
fix/disable-repo-interop-test
Apr 20, 2020
Merged

fix: all repo compatibility tests#109
achingbrain merged 1 commit into
masterfrom
fix/disable-repo-interop-test

Conversation

@Stebalien

@Stebalien Stebalien commented Mar 23, 2020

Copy link
Copy Markdown
Member

We've changed how we name keys in the keystore but the js-ipfs migration isn't quite ready. The real issue here is that the repo versions differ.

@achingbrain

achingbrain commented Mar 23, 2020

Copy link
Copy Markdown
Member

If unskipped they fail with:

  1) ipns locally using the same repo across implementations
       should publish an ipns record to a js daemon and resolve it using a go daemon through the reuse of the same repo:
     Error: Timeout of 160000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/circleci/ipfs/go-ipfs/interop/test/ipns.js)
  

  2) ipns locally using the same repo across implementations
       should publish an ipns record to a go daemon and resolve it using a js daemon through the reuse of the same repo:
     Error: Timeout of 160000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/circleci/ipfs/go-ipfs/interop/test/ipns.js)

@vasco-santos any ideas?

@Stebalien

Copy link
Copy Markdown
Member Author

Go PR: ipfs/kubo#6955

@vasco-santos

Copy link
Copy Markdown
Member

We currently use this format: https://github.com/ipfs/js-ipns/blob/master/src/index.js#L209-L219

I need to try it out and look on go changes, but cannot do it today

@Stebalien

Copy link
Copy Markdown
Member Author

@vasco-santos we haven't changed the way we're storing IPNS records, we've changed the way we encode key names in the on-disk keystore. As far as I know, that shouldn't affect this test as we're using the identity key (which isn't stored in the keystore, IIRC).

@vasco-santos

Copy link
Copy Markdown
Member

@Stebalien I understood and I think that the problem might be the key name stored in the local datastore. We store locally here the IPNS record, using the datastore key in this format.

For what I understand, the naming should have changed and js-ipfs cannot get it from the datastore, in order to get the IPNS records. I need to check what is going on later to be more accurate

@Stebalien

Copy link
Copy Markdown
Member Author

I'm lost. We haven't changed anything about IPNS records and/or how we store/name them. We've only changed how we encode the names of private keys.

@hsanjuan

Copy link
Copy Markdown
Contributor

Isn't the js daemon just failing to start because of the different repo version when using same repo?

[23:11:33] ~/n/.bin $ ./jsipfs daemon
Initializing IPFS daemon...
js-ipfs version: 0.41.2
System version: x64/linux
Node.js version: 12.16.1
Incompatible repo version. Migration needed. Pass --migrate for automatic migration

@Stebalien

Copy link
Copy Markdown
Member Author

Oh. My bad. That explains it.

@Stebalien Stebalien force-pushed the fix/disable-repo-interop-test branch from 8a59ce2 to 77e29c8 Compare March 24, 2020 00:08
We've changed how we name keys in the keystore but the js-ipfs migration isn't
quite ready.
@Stebalien Stebalien force-pushed the fix/disable-repo-interop-test branch from 77e29c8 to cebaa1b Compare March 24, 2020 01:31
@Stebalien Stebalien changed the title fix: disable an IPNS interop tests that depends on repo compatibility fix: all repo compatibility tests Mar 24, 2020
@Stebalien

Copy link
Copy Markdown
Member Author

I've changed this to disable all repo compatibility tests as they all fail now.

@vasco-santos vasco-santos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Seems a good call until js-ipfs updates the repo version👍

@achingbrain achingbrain left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, just needs green CI

@achingbrain

Copy link
Copy Markdown
Member

What feature is the repo version bump in go for?

@Stebalien

Copy link
Copy Markdown
Member Author
  1. Fixing the bootstrap peers in the config to remove the 1024bit peers, make sure we have the new 2048bit peers, and replace all instances of /ipfs/ with /p2p/.
  2. Encoding keys in the on-disk keystore using base32: feat: encoding key's name to base32 js-ipfs#2410

@hsanjuan

hsanjuan commented Apr 6, 2020

Copy link
Copy Markdown
Contributor

should this be merged?

@achingbrain

Copy link
Copy Markdown
Member

CI is still failing, that should be resolved before merging

@Stebalien

Copy link
Copy Markdown
Member Author

The CI issue is unrelated. Force-pushing to rerun.

@Stebalien

Copy link
Copy Markdown
Member Author

@achingbrain it looks like the firefox circuit tests are failing for unrelated reasons.

@achingbrain

Copy link
Copy Markdown
Member

The firefox circuit tests have been fixed in master by upgrading libp2p-webrtc-star as part of #108 - ref: libp2p/js-libp2p-webrtc-star#212

@achingbrain achingbrain merged commit edfaa09 into master Apr 20, 2020
@achingbrain achingbrain deleted the fix/disable-repo-interop-test branch April 20, 2020 09:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants