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

Bugfix daemon #718

Merged
merged 9 commits into from
Jul 20, 2023
Merged

Bugfix daemon #718

merged 9 commits into from
Jul 20, 2023

Conversation

gador
Copy link
Contributor

@gador gador commented Sep 28, 2020

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.

it utilizes a settings class (called App) to store settings in.
Right now, this only uses the daemon API address.
For the webui it confirmes a local running daemon,
before trying to issue start and stop commands.
@codecov
Copy link

codecov bot commented Sep 28, 2020

Codecov Report

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

❗ 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%) ⬆️
ipwb/__main__.py 22.58% <53.33%> (+22.58%) ⬆️
ipwb/util.py 44.87% <61.53%> (+1.31%) ⬆️
ipwb/settings.py 83.33% <76.92%> (+83.33%) ⬆️
ipwb/backends.py 91.48% <100.00%> (ø)

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

@machawk1
Copy link
Member

machawk1 commented Oct 8, 2020

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.

@gador
Copy link
Contributor Author

gador commented Oct 9, 2020

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.

@machawk1 machawk1 self-requested a review October 15, 2020 16:35
Copy link
Member

@machawk1 machawk1 left a comment

Choose a reason for hiding this comment

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

LGTM. Waiting on @ibnesayeed to review before merging.

@machawk1
Copy link
Member

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

@machawk1 machawk1 merged commit 2ba5ea9 into oduwsdl:master Jul 20, 2023
27 checks passed
@machawk1
Copy link
Member

Thanks again for your contribution, @gador.

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.

Daemon CLI argument not implemented
2 participants