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.

Sign up for GitHub

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

Jump to bottom

[WIP] docs: add example for non disposable daemons #304

Closed
wants to merge 4 commits into from
Closed

[WIP] docs: add example for non disposable daemons #304

wants to merge 4 commits into from

Conversation

daviddias
Copy link
Member

The non-disposable daemons option is currently not working. It errors silently.

Steps to reproduce:

    1. clone this branch, npm install, go to examples/local-non-disposable and run npm install for it to link the main module
    1. run ipfs init and jsipfs-init
    1. node index.js

Result

image

It should spawn a go and js daemons on top of .ipfs and .jsipfs respectively and run the .id call without a problem.

@daviddias daviddias added the kind/bug A bug in existing code (including security flaws) label Oct 24, 2018
@ghost ghost assigned daviddias Oct 24, 2018
@ghost ghost added the status/in-progress In progress label Oct 24, 2018
@hugomrdias
Copy link
Member

You need to init and start, check the link below
https://github.com/ipfs/js-ipfsd-ctl#disposable-vs-non-disposable-nodes

@daviddias
Copy link
Member Author

Why not autostart ou autoinit if neither have happened so far??

@hugomrdias
Copy link
Member

I would think whoever introduce this behaviour, tried to make disposable fully automated and non-disposable the flexible version. You just need to pass .spawn {init: true, start:true}

It's just the way it works and is documented right now, we can make an issue about it and change the behaviour.

@daviddias
Copy link
Member Author

You just need to pass .spawn {init: true, start:true} This is not working for me either (even if I delete init, given that I already have my repo init'ed)

@hugomrdias
Copy link
Member

hugomrdias commented Oct 24, 2018 •
edited
Loading

That is bug then! can you make an issue about it pls? i'll look into it ASAP

@daviddias daviddias mentioned this pull request Oct 24, 2018
Open
@daviddias
Copy link
Member Author

Can we treat this PR as the issue? Or you prefer me to open another issue?

@hugomrdias hugomrdias mentioned this pull request Oct 24, 2018
Closed
@hugomrdias hugomrdias removed the kind/bug A bug in existing code (including security flaws) label Oct 24, 2018
daviddias
daviddias commented Oct 24, 2018
View reviewed changes
src/factory-daemon.js Outdated
(cb) => options.init
? node.init(options.initOptions, cb)
: cb(null, node),
// TODO if start fails, check if it was because there
// is a daemon already running and connect to it instead
Copy link
Member Author

Choose a reason for hiding this comment

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

Just needs a bunch of checks here :)

daviddias
daviddias commented Oct 24, 2018
View reviewed changes
src/factory-daemon.js
@@ -98,8 +98,6 @@ class FactoryDaemon {

if (!options.disposable) {
const nonDisposableConfig = clone(defaultConfig)
options.init = false
options.start = false
Copy link
Member Author

Choose a reason for hiding this comment

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

this was what was making it not start even if passed the options

@daviddias daviddias added kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up labels Oct 25, 2018
@daviddias daviddias mentioned this pull request Oct 27, 2018
Closed
@daviddias daviddias changed the title docs: add example for non disposable daemons fix: Make non-disposable daemons work Oct 27, 2018
@daviddias daviddias changed the title fix: Make non-disposable daemons work [WIP] docs: add example for non disposable daemons Oct 27, 2018
daviddias
daviddias commented Oct 28, 2018
View reviewed changes
src/factory-daemon.js
if (err) {
console.log(err)
// TODO if start fails, check if it was because there
// is a daemon already running and connect to it instead
Copy link
Member Author

Choose a reason for hiding this comment

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

Would appreciate some brainstorming on this one. Would the best just to move this func out https://github.com/ipfs/js-ipfsd-ctl/blob/master/src/ipfsd-daemon.js#L229-L234 and if the daemon is running, catch the addr from the api file in the repo?

@hugomrdias?

@hugomrdias hugomrdias mentioned this pull request Oct 29, 2018
Closed
@hugomrdias
Copy link
Member

@diasdavid can you check #308 related to this.

@daviddias daviddias added P2 Medium: Good to have, but can wait until someone steps up and removed P1 High: Likely tackled by core team if no one steps up labels Nov 5, 2018
@daviddias daviddias added the exp/expert Having worked on the specific codebase is important label Nov 5, 2018
@hugomrdias hugomrdias mentioned this pull request Sep 17, 2019
Merged
@hugomrdias
Copy link
Member

doesn't apply anymore version 1.0.0 fixed this problem.

@hugomrdias hugomrdias closed this Jan 16, 2020
@hugomrdias hugomrdias deleted the non-disposable-example branch January 16, 2020 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@daviddias daviddias

Labels
exp/expert Having worked on the specific codebase is important kind/bug A bug in existing code (including security flaws) P2 Medium: Good to have, but can wait until someone steps up status/in-progress In progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@daviddias @hugomrdias