Skip to content
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

Temporarily install ipfshttpclient from master branch #641

Merged
merged 3 commits into from
Apr 29, 2020
Merged

Conversation

ibnesayeed
Copy link
Member

@ibnesayeed ibnesayeed commented Apr 28, 2020

A recent change in the IPFS API removed support for GET/HEAD methods on some endpoints. This prevents us from upgrading to go-ipfs-v0.5.0 as per #638. Corresponding change in the ipfshttpclient is added to the master branch, but not released on PyPI yet. This is a temporary fix, but once the updated package is out, we should update the dependency.

Since this is a temporary fix, I am not changing the dependency in the setup.py file. Should I?

@codecov
Copy link

codecov bot commented Apr 28, 2020

Codecov Report

Merging #641 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #641   +/-   ##
=======================================
  Coverage   27.28%   27.28%           
=======================================
  Files           7        7           
  Lines        1246     1246           
  Branches      191      191           
=======================================
  Hits          340      340           
  Misses        881      881           
  Partials       25       25           
Impacted Files Coverage Δ
setup.py 0.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4adb8bd...070ef18. Read the comment docs.

@ibnesayeed
Copy link
Member Author

ibnesayeed commented Apr 29, 2020

I made changes in the setup.py file as well, but now it is complaining about lne too long, thought I would not like to break URL to pass the linter.

@ibnesayeed
Copy link
Member Author

Thanks for this @machawk1, yesterday I was looking for annotations to add exception here, but I did not search well enough. From the brief look at the documentation I thought they have options to disable certain validations globally, but not per line exceptions.

@machawk1
Copy link
Member

@ibnesayeed LGTM as a temp fix. I added a directive to the setup file in 070ef18 to allow the line to pass the linter. In an effort to stay current, should we push another release for compatibility's sake?

@machawk1 machawk1 merged commit e4b1088 into master Apr 29, 2020
@ibnesayeed
Copy link
Member Author

In an effort to stay current, should we push another release for compatibility's sake?

Let's wait for the new release of the ipfshttpclient before we push a new release. It is our primary dependency and I do not feel comfortable tagging a release that has non-frozen dependencies. Also, I am not too confident that PyPI will install the dependency properly when releasing there because there have been some changes in how dependencies are specified from a remote resource in the setup.py file. However, if the ipfshttpclient takes forever to release the new version, we may revisit our plan after a while.

@machawk1
Copy link
Member

Ok, I agree, but the latest release of ipwb does not work with the latest release of go-ipfs currently. Do you want to revisit this in a few days, @ibnesayeed, and push a new ipwb release with a reference to the ipfshttpclient master if ipfshttpclient has not released a new version by then? Should we pin to an ipfshttpclient commit instead of simply master? Pinning to master seems dangerous and potentially breaking.

@ibnesayeed
Copy link
Member Author

If we were to release in a few days, pinning to a hash will be safer, but not all intermediary references are fetchable from Git servers (a configuration for efficiency). We can perhaps ask the ipfshttpclient maintainer about their next release plan.

@machawk1
Copy link
Member

@ibnesayeed Can you take the lead on the task you mentioned? Feel free to reference the relevant GitHub issues in the ipwb repo.

@ibnesayeed
Copy link
Member Author

Can you take the lead on the task you mentioned?

Posted a comment at ipfs-shipyard/py-ipfs-http-client#204 (comment) to inquire about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants