ipwb icon indicating copy to clipboard operation
ipwb copied to clipboard

Bugfix daemon

Open gador opened this issue 4 years ago • 4 comments

This bugfix adds an implementation for the daemon address and therefore closes #717

it utilizes a settings class (called App) to store settings in.This is necessary, because the flask web app needs to know the daemon address (and can't easily be passed). Also, passing the daemon API address as an argument to every function is cumbersome.

Right now, the settings class only uses the daemon API address, but could be extended to possibly separate the .ipfs/config from ipwb.

Also adds checks for the WebUI to only attempt to control locally running daemons.

gador avatar Sep 28 '20 13:09 gador

Codecov Report

Merging #718 (b4b1eb5) into master (ae2e574) will increase coverage by 2.47%. The diff coverage is 61.22%.

:exclamation: Current head b4b1eb5 differs from pull request most recent head e59c532. Consider uploading reports for the commit e59c532 to get more accurate results

@@            Coverage Diff             @@
##           master     #718      +/-   ##
==========================================
+ Coverage   30.08%   32.56%   +2.47%     
==========================================
  Files          10       10              
  Lines        1263     1296      +33     
  Branches      185      186       +1     
==========================================
+ Hits          380      422      +42     
+ Misses        860      849      -11     
- Partials       23       25       +2     
Impacted Files Coverage Δ
ipwb/replay.py 16.15% <33.33%> (+0.25%) :arrow_up:
ipwb/__main__.py 22.58% <53.33%> (+22.58%) :arrow_up:
ipwb/util.py 44.87% <61.53%> (+1.31%) :arrow_up:
ipwb/settings.py 83.33% <76.92%> (+83.33%) :arrow_up:
ipwb/backends.py 91.48% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov[bot] avatar Sep 28 '20 14:09 codecov[bot]

Hi @gador, thanks for the PR! In discussing your additions with @ibnesayeed, we wondered if there was a reason that you used a class to store the attributes in settings.py rather than use it as a simple dictionary? Having to retrieve values through settings.App.get('attr') rather than something like settings['attr'] seems verbose.

Regardless, your changes are very welcomed in removing the need to pass around the daemon multiaddress and toward our move to not have to store the ipwb values in the IPFS config solely for retrieval around the code.

machawk1 avatar Oct 08 '20 20:10 machawk1

hi @machawk1 and @ibnesayeed , thanks for having a look!

I was looking around how to properly implement settings in python and stumbled upon this StackOverflow answer.

One of the main reasons to use a class for me, is that it avoids global variables. It uses a separate get and set method and is in itself a bit cleaner.

I read the new issue #719 and found the python implementation of multiaddr. I added a way to use the class to validate the entered multiaddr, so we won't have to do it manually. I also added some rudimentary tests.

I'm also working on a way to remove the ipfs config dependency and habe a working version on my pc (and on my github page under this branch).

If this PR with the daemon_address looks alright to you, I'll rebase the feature_ipfs_config branch on it and submit a new PR.

gador avatar Oct 09 '20 10:10 gador

Ping @ibnesayeed. This PR was submitted a month ago and is awaiting your approval and/or comment.

machawk1 avatar Oct 28 '20 18:10 machawk1

Thanks again for your contribution, @gador.

machawk1 avatar Jul 20 '23 20:07 machawk1