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

Fixes #4522 #4552

Open
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@acolytec3
Copy link
Contributor

commented May 31, 2019

Description

Replace deprecated ifpsapi library with ipfshttpclient in dashboard, kudos, and healthcheck.. Add tests for dashboard.utils that interact with IPFS.

Refers/Fixes

Fixes #4522'

Testing

Added tests for get_ipfs and ipfs_cat_ipfsapi in dashboard.utils to verify connectivity and retrieval of objects by IPFS hash string are working.

acolytec3
@codecov

This comment has been minimized.

Copy link

commented May 31, 2019

Codecov Report

Merging #4552 into master will decrease coverage by 0.03%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4552      +/-   ##
==========================================
- Coverage   30.17%   30.13%   -0.04%     
==========================================
  Files         209      209              
  Lines       16886    16886              
  Branches     2278     2278              
==========================================
- Hits         5095     5089       -6     
- Misses      11594    11600       +6     
  Partials      197      197
Impacted Files Coverage Δ
app/kudos/utils.py 20.37% <50%> (ø) ⬆️
app/dashboard/utils.py 34.49% <66.66%> (ø) ⬆️
app/dashboard/embed.py 28.16% <0%> (-3.45%) ⬇️

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 4ec5cca...50c1102. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented May 31, 2019

Codecov Report

Merging #4552 into master will decrease coverage by 0.05%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4552      +/-   ##
==========================================
- Coverage   30.17%   30.12%   -0.06%     
==========================================
  Files         209      213       +4     
  Lines       16886    17097     +211     
  Branches     2278     2311      +33     
==========================================
+ Hits         5095     5150      +55     
- Misses      11594    11746     +152     
- Partials      197      201       +4
Impacted Files Coverage Δ
app/healthcheck/healthchecks.py 40% <0%> (ø) ⬆️
app/dashboard/utils.py 37.12% <100%> (+2.63%) ⬆️
app/kudos/utils.py 20.37% <33.33%> (ø) ⬆️
app/grants/management/commands/subminer.py 42.99% <0%> (-2.56%) ⬇️
app/kudos/models.py 54.75% <0%> (-2.05%) ⬇️
app/dashboard/admin.py 65.92% <0%> (-0.75%) ⬇️
app/grants/models.py 49.67% <0%> (-0.51%) ⬇️
app/kudos/views.py 21.55% <0%> (-0.29%) ⬇️
app/dashboard/helpers.py 13.96% <0%> (-0.22%) ⬇️
app/retail/utils.py 10.34% <0%> (-0.09%) ⬇️
... and 18 more

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 4ec5cca...5463a63. Read the comment docs.

@acolytec3

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

Hold on this. I thought everything was working and now I'm seeing some errors in my local build.

acolytec3
@acolytec3

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2019

Appears to be working now that I've corrected the connect parameters. I'm not seeing the connection errors that surfaced previously so should be ready for review.

@acolytec3

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@owocki @kuhnchris Any chance I could get this reviewed? Would love to get it closed so I can move on a look at applying for some other bounties.

Thanks!

@kuhnchris

This comment has been minimized.

Copy link
Collaborator

commented Jun 7, 2019

@danlipert do you have time to review this? Else I'll take a look

@danlipert

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

@kuhnchris @acolytec3 The existing tests are pretty lackluster and don't really test IPFS. I'd like some a better writeup of how this change was tested and what the differences are between this library and the newer one. Is it simply a rename? Since there are compatibility issues with Python 3.4 I'm guessing its not.

@acolytec3

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

Here's the official write-up on the changes from the old library.

Based on reading the docs, the only change that's pertinent for how gitcoin uses IPFS is the connect method and I've updated that to match the new structure in both of the places in the code where it's called.

The other functions from the library that are used (cat, get_json, add_json) appear to be unchanged.

I'll look at writing some tests for those specific utils methods where connect is used.

acolytec3 added some commits Jun 13, 2019

@acolytec3

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

@danlipert I added tests for both of the dashboard utility methods that call ipfs. Let me know if this is what you're looking for.

And don't mind my complete ineptitude at squashing commits. ^^;;

@danlipert
Copy link
Collaborator

left a comment

@acolytec3 Looks good! Did you try running the app locally and interacting with IPFS via the backend?

Show resolved Hide resolved app/dashboard/utils.py
@acolytec3

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2019

@danlipert danlipert requested review from thelostone-mc and octavioamu Jun 28, 2019

@acolytec3

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

@danlipert @octavioamu @thelostone-mc @owocki Hi all, what's the status on this one? Are there more tests or validations you're looking for to close this out?

@owocki

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

quick question.. are PRs that are approved included in releases automatically, or is there some other inclusion metric?

@danlipert

This comment has been minimized.

Copy link
Collaborator

commented Jul 16, 2019

quick question.. are PRs that are approved included in releases automatically, or is there some other inclusion metric?

Once they are approved by each engineer, then the final approving engineer usually merges it. Sometimes we save merging until before the deploy in case there are conflicts, especially with migrations.

@thelostone-mc @octavioamu this look good to y'all?

@acolytec3

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

What about this one? Is it good to go? Would love to get paid if it's done. 😬

@octavioamu
Copy link
Collaborator

left a comment

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.