-
Notifications
You must be signed in to change notification settings - Fork 40
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
Bugfix daemon #718
Conversation
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 Report
@@ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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. |
hi @machawk1 and @ibnesayeed , 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. |
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.
LGTM. Waiting on @ibnesayeed to review before merging.
Ping @ibnesayeed. This PR was submitted a month ago and is awaiting your approval and/or comment. |
Thanks again for your contribution, @gador. |
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.