-
Notifications
You must be signed in to change notification settings - Fork 242
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
Update to ipfs v0.5.0 #170
Conversation
(use POST instead of GET)
(pin update switched to multihash rather than multiaddr return)
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.
Thank you @ianopolous, yes, iiuc this ticks all the boxes from #157:
- Support for POST-only HTTP API from go-ipfs 0.5+
- Support for CIDv1 in Base32 and Base36
- Fix tests and CI
Before we merge and close the bounty, some small questions/asks inline.
ps. just to have another set of eyes familiar with Java,
@NadeeraM are you able to check if this branch works for you too?
Co-authored-by: Marcin Rataj <[email protected]>
This comment has been minimized.
This comment has been minimized.
That looks like maven is still using the old dependencies. I don't actually use maven myself, but I'll have a look. |
@NadeeraM I've just fixed the maven file I think. Are you able to try again? |
@ianopolous yes I will check |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@NadeeraM jitpack.io had got itself into a strange state, but I've bumped a few versions in the dependencies for maven and it is working now. |
@ianopolous yes can compile the code successfully. But Still tests are failing. This is the tests that fail
|
The tests all run fine in CI, and if I run them locally using maven against ipfs v0.5.0 then they all pass:
The errors you're getting, @NadeeraM look like your running against an older version of ipfs, 0.4.23 or earlier (when pin update returned multiaddr rather than multihashes). Could you confirm the ipfs version you are testing against? |
@ianopolous Yes i am running local ipfs using docker-compose file in this project. Version is "Version": "0.4.16".
Thank you |
@lidel Any other comments on this? I think it's good to go now. |
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.
@ianopolous I noticed tests did not run against bundled junit and cid.jar
(?) and after adding both in f092411 CI no longer passed (example).
I've re-run the job and it is now green – any idea why previous run failed? Anything we can do to make tests more deterministic, or at least tell us what exactly failed?
Instead use 95% delivery of messages
It includes the cid jar via the ipfs.jar manifest.
Some of the tests are not strict unit tests because they impose a time limit and are thus more of a performance test of ipfs, like the pubsubSynchronous test. I've rewritten that to ignore the time now and instead require 95% delivery of messages (ipfs pubsub isn't reliable even on the same instance it seems). |
Thank you @ianopolous, expecting 95% delivery of messages should work better than time-based one 👍 👉 I've confirmed this PR works for me locally and satisfies requirements of both bounties. We will finalize bounty details in #157 |
This fixes all the breaking changes in ipfs v0.5.0.
As well as upgrading dependencies.
@lidel Does this qualify for the two bounties?