Complete revamp! Spawn js/go daemons locally or remote (from the browser) #176

dryajov · 2017-11-22T21:56:16Z

In order to merge this PR, we need a working PR for:

bring back test coverage to 90%
ipfs-station feat: js-ipfsd-ctl daemon-remote ipfs-desktop#549 (thanks to @hacdias )
js-ipfs-api feat: use new ipfsd-ctl ipfs-inactive/js-ipfs-http-client#652
ipfs/interop tests feat: port to new ipfsd-ctl interop#5
interface-ipfs-core feat: use new ipfsd-ctl ipfs-inactive/interface-js-ipfs-core#186
js-ipfs feat: reworking tests with new ipfsd-ctl js-ipfs#1167

Note: This is a major blocker for the circuit effort, let's timebox a decision on this interface for the next 24h (Nov 28), if there are no objections or comments I'll go ahead with the proposed interface.

This is still mostly a POC. I'm not sure whether this is the direction we want to take, so I'm leaving it here as a starting point to help understand (for myself mostly) what we're looking for.

Basically, I added a factory that aids in spawning IPFS nodes (js and go) from the cli and the browser. The factory has the same interface as https://github.com/ipfs/js-ipfs/blob/master/test/utils/ipfs-factory-daemon/index.js, essentially spawnNode and dismatle methods. This is the de facto interface we're using in js-ipfs tests.

Currently, my questions are

Should we change ipfsd-ctl interface completely to just being ipfs-factory-daemon? If we do that, we break this modules (from npmjs)
- ipfs-daemon
- ipscend
- orbit-common
- grunt-ipfs
- node-ipfs-mirror
- beaker-plugin-ipfs
- dapple-pkg
Should we go with the current approach and just have a helper factory that will be context aware (cli vs browser) and do the right thing?
Something else completely?

@victorbjelkholm @diasdavid @dignifiedquire @haadcode

EDIT: added below section with proposed interface

To add more clarity to this issue, this is what I currently have in mind:

ipfsd-ctl is a module intended to allow interacting with external ipfs processes. It should allow spawning, configuring and stopping ipfs nodes, for both js-ipfs and go-ipfs. Currently this module already accomplishes that, however, its current interface seems to be a bit convoluted and in need of rethinking.

In addition to this, we have several other needs, such as testing, that require manipulating ipfs nodes in an easy and convenient manner from both node and browsers (as well as electron and other such environments).

The current interface looks like this

  /**
   * Create a new local node.
   *
   * @memberof IpfsDaemonController
   * @param {string} [path] - Location of the repo. Defaults to `$IPFS_PATH`, or `$HOME/.ipfs`, or `$USER_PROFILE/.ipfs`.
   * @param {Object} [opts={}]
   * @param {function(Error, Node)} callback
   * @returns {undefined}
   */
  local (path, opts, callback)

Create a new, uninitialized node.

  /**
   * Create a new disposable node.
   * This means the repo is created in a temporary location and cleaned up on process exit.
   *
   * @memberof IpfsDaemonController
   * @param {Object} [opts={}]
   * @param {function(Error, Node)} callback
   * @returns {undefined}
   */
  disposable (opts, callback)

Create a new node and initialize a repo in a temporary location, starting/stopping is up to the client. Alternatively, although the the docs don't mention that, the repo can be overridden.

  /**
   * Create a new disposable node and already started the daemon.
   *
   * @memberof IpfsDaemonController
   * @param {Object} [opts={}]
   * @param {function(Error, Node)} callback
   * @returns {undefined}
   */
  disposableApi (opts, callback)

Create a new node, initialize a repo in a temporary location and return an ipfs-api connected to that node. Alternatively, although the the docs don't mention that, the repo can be overridden.

The above is the factory interface that gets exposed to consumers that returns either a Node instance in the case of local and disposable or an ipfs-api instance in the case of disposableApi.

What we want

EDIT:

updated the options param with actual options.
updated spawn interface with comments from Complete revamp! Spawn js/go daemons locally or remote (from the browser) #176 (comment)

A simplified interface that allows starting and stopping nodes

Spawn Nodes

  /**
    Spawn an IPFS node, either js-ipfs or go-ipfs

    @param {Integer} [count=1] - the amount of nodes to start, defaults to 1
    @param {Object} [options] - various config options and ipfs config parameters (see valid options below)
    @param {Function} cb(err, [`ipfs-api instance`, `Node (ctrl) instance`]) - a callback that receives an array with an `ipfs-instance` attached to the node and a `Node` (https://github.com/ipfs/js-ipfsd-ctl/blob/master/src/daemon.js) instance to control the spawned node
  */
  spawn(count, options, cb)

Where options is:

js bool - spawn a js or go node (default go)
remote bool - spawn a local or remote node (default false)
- If remote is chosen, nodes are spawned using websockets
repoPath string - the repository path to use for this node, ignored if node is disposable
disposable bool - a new repo is created and initialized for each invocation
config - ipfs configuration options

DEPRECATED
Shutdown Nodes

  /**
    Shutdown an IPFS node, either js-ipfs or go-ipfs

    @param {Array} [ids=null] - an optional array of ids to shutdown, if empty, shutdown all of the currently spawned nodes, otherwise those that are listed
    @param {Function} cb(err) - callback, called after all passed nodes are shut down
  */
  shutdown([ids], cb)

In addition to this, this factory should be consumable from browsers, our current requirement for this are browser test, that are crippled by the absence of this functionality.

richardschneider · 2017-11-23T13:02:20Z

test/spawning-js-daemons.spec.js

+  })
+
+  describe('setting up and init a local node', () => {
+    const testpath1 = '/tmp/ipfstestpath1'


'/tmp' is a nogo windows, use os.tmpdir and path.join

richardschneider · 2017-11-23T13:03:09Z

test/spawning-js-daemons.spec.js

+
+describe('daemon spawning', () => {
+  describe('local daemon', () => {
+    const repoPath = '/tmp/ipfsd-ctl-test'


use os.tmpdir()

richardschneider · 2017-11-23T13:05:07Z

test/spawning-js-daemons.spec.js

+
+          // actually running?
+          done = once(done)
+          exec('kill', ['-0', pid], { cleanup: true }, () => done())


will this work on windows?

richardschneider · 2017-11-23T13:05:56Z

test/spawning-js-daemons.spec.js

+      })
+
+      it('should not have a directory', () => {
+        expect(fs.existsSync('/tmp/ipfstestpath1')).to.be.eql(false)


os.tmpdir()

dryajov · 2017-11-23T13:55:23Z

Thanks @richardschneider! This will probably end up being reworked quite a bit, that's why I won't make any changes until we settle on what we want this module to do for us! I'll take your observations into account tho! 👍

dryajov · 2017-11-27T16:16:41Z

After a convo with @diasdavid on IRC, this interface was proposed:

09:31 <daviddias> one good thing to add would be that spawn would give you an object with { ctl: <ipfs-api instance controlling the daemon>, ctrl: <direct control to the daemon, things like shutdown, cleanUp>}

The only issue I see with returning the node is when running in remote mode (starting nodes over http/ws), which will require exposing Node (aka ctrl) methods as http/ws endpoints. I like this approach because it gives us a consistent interface for remote and local nodes, and I think we should got with it.

dryajov · 2017-11-29T17:48:46Z

I've began reworking this PR with the proposed interface.

daviddias · 2017-12-01T08:16:55Z

package.json

-    "test": "aegir test -t node --timeout 50000",
+    "test": "aegir test -t node -t browser --no-cors --timeout 50000",
+    "test:node": "aegir test -t node --timeout 50000",
+    "test:browser": "aegir test -t browser --no-cors --timeout 50000",


While you are at it, please remove global timeouts. Thank you :)

Powersource · 2017-12-01T14:04:54Z

Looks nice! I've wanted to use non-disposable nodes but had some issues with that before, this api looks cleaner (and already more documented :) ).

How does it handle shutdown? (how did it handle it before?) Seeing the shutdown function deprecated (I didn't realize there was one before) I'm guessing the daemon shuts down gracefully in the backgrund somehow when this api is closed/GC'd?

dryajov · 2017-12-01T23:34:01Z

@Powersource Thanks!

I also think this is going to be more concise than what we had previously 😉 . As for shutdown, that was part of the initially proposed interface, but we went a slightly different route and instead return a controlling node which will allow you to call startDaemon and stopDaemon as well as various other methods, you'll also get an ipfs-api instance back if the spawn is called with no parameters, which will init and start a disposable node as well as attach the ipfs api to it - {ctl: [ipfs-api], ctrl: controlling node}.

codecov · 2017-12-06T17:50:14Z

Codecov Report

Merging #176 into master will decrease coverage by 0.87%.
The diff coverage is 89.89%.

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
- Coverage   89.84%   88.96%   -0.88%     
==========================================
  Files           3       11       +8     
  Lines         197      553     +356     
==========================================
+ Hits          177      492     +315     
- Misses         20       61      +41

Impacted Files	Coverage Δ
src/remote-node/index.js	`100% <100%> (ø)`
src/index.js	`75% <75%> (-19.29%)`	⬇️
src/in-proc-node.js	`80.76% <80.76%> (ø)`
src/remote-node/routes.js	`89.61% <89.61%> (ø)`
src/daemon-ctrl.js	`90.69% <90.69%> (ø)`
src/utils/index.js	`91.37% <91.37%> (ø)`
src/daemon-node.js	`85.36% <92.3%> (ø)`
src/remote-node/server.js	`93.33% <93.33%> (ø)`
src/remote-node/client.js	`94.79% <94.79%> (ø)`
src/utils/create-repo-nodejs.js	`95.23% <95.23%> (ø)`
... and 10 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 3c328de...c35f6ce. Read the comment docs.

dryajov · 2017-12-06T18:07:11Z

test/exec.js

-describe.skip('exec', () => {
+// UPDATE: 12/06/2017 - `tail` seems to work fine on all ci systems.
+// I'm leaving it enabled for now. This does need a different approach for windows though.
+describe('exec', () => {
  it('SIGTERM kills hang', (done) => {


@richardschneider don't think tail would work on windows, any ideas on how to make it windows compat?

dryajov · 2017-12-07T05:57:28Z

A small update:

The interface has been reworked and the controller is now capable of running both Go and JS ipfs nodes as well as running in the browser. I want to clean this up a little before removing the WIP - this should happen shortly.

dryajov · 2017-12-07T21:16:05Z

This requires - ipfs/js-ipfs#1134

dryajov · 2017-12-08T00:11:01Z

@diasdavid this should be ready for review.

daviddias

Did my first round of review. Lot's of great work here! Thank you @dryajov :)

daviddias · 2017-12-10T10:09:02Z

README.md


 // IPFS_PATH will point to /tmp/ipfs_***** and will be
 // cleaned up when the process exits.

-var ipfsd = require('ipfsd-ctl')
+const factory = require('ipfsd-ctl')


s/factory/daemonFactory

daviddias · 2017-12-10T10:09:31Z

README.md


 // IPFS_PATH will point to /tmp/ipfs_***** and will be
 // cleaned up when the process exits.

-var ipfsd = require('ipfsd-ctl')
+const factory = require('ipfsd-ctl')
+const localController = factory.localController


s/localController/local

daviddias · 2017-12-10T10:09:54Z

README.md

@@ -30,23 +30,92 @@ npm install --save ipfsd-ctl

 IPFS daemons are already easy to start and stop, but this module is here to do it from JavaScript itself.


Needs a diagram explaining the difference between a local spawn and a remote spawn and why both exist

daviddias · 2017-12-10T10:10:36Z

README.md


-ipfsd.disposableApi(function (err, ipfs) {
+localController.spawn(function (err, ipfsd) {
+  const ipfs = ipfsd.ctl


s/const ipfs/const ipfsCtl/

daviddias · 2017-12-10T10:11:56Z

README.md

-ipfsd.disposableApi(function (err, ipfs) {
+localController.spawn(function (err, ipfsd) {
+  const ipfs = ipfsd.ctl
+  const node = ipfsd.ctrl


s/node/ipfsCtrl

Essentially, avoid confusing users for having examples that literally rename what the API is giving you. It is like saying "the name it gives is misleading" when in fact it is not.

great point! will rename to something more meaningful.

daviddias · 2017-12-10T10:23:25Z

src/daemon.js

@@ -78,30 +78,38 @@ function parseConfig (path, callback) {
  ], callback)
 }

+function tempDir (isJs) {
+  return join(os.tmpdir(), `${isJs ? 'jsipfs' : 'ipfs'}_${String(Math.random()).substr(2)}`)


Please use the hat module http://npmjs.com/hat

daviddias · 2017-12-10T10:23:44Z

src/daemon.js

+function tempDir (isJs) {
+  return join(os.tmpdir(), `${isJs ? 'jsipfs' : 'ipfs'}_${String(Math.random()).substr(2)}`)
+}
+
 /**
 * Controll a go-ipfs node.
 */
 class Node {


Rename the class to GoDaemon

I forgot to change the comment, this now controls both, go and js nodes.

daviddias · 2017-12-10T10:24:11Z

src/daemon.js

-    this.exec = process.env.IPFS_EXEC || ipfsDefaultPath
+  constructor (opts) {
+    const rootPath = process.env.testpath ? process.env.testpath : __dirname
+    const isJs = truthy(process.env.IPFS_JS)


Weird, if the class says it is to control a gonode, why it is checking JS stuff?

same as above, controls both, go and js nodes.

daviddias · 2017-12-10T10:25:22Z

src/local.js

+    'Addresses.Swarm': [`/ip4/127.0.0.1/tcp/0`],
+    'Addresses.API': `/ip4/127.0.0.1/tcp/0`,
+    'Addresses.Gateway': `/ip4/127.0.0.1/tcp/0`
+  },


Shouldn't all of these be the default options of IPFS? Do we need to redefine them?

This is so that we can start multiple nodes on different random ports, otherwise it uses the default ports and spawning multiple nodes ends up conflicting.

So perhaps this should be the default for disposable nodes? It should also be one of the options that gets passed into spawn.

daviddias · 2017-12-10T10:26:51Z

test/spawning.js

+        })
+      })
+
+      describe('validate api', () => {


Break this set of tests into its own file.

dryajov · 2017-12-11T18:54:12Z

Adding some todos based on @diasdavid review:

Update circle ci build config
Breakout api tests into its own file
Fix examples to use ipfsd-ctl as a dependency in package.json
Fix README with provided suggestions
Create a diagram explaining the difference between a local spawn and a remote spawn and why both exist
Fix electron example
Fix defaults for disposable nodes
Allow passing configs as object instead of strings

daviddias · 2017-12-13T17:41:46Z

README.md


 // IPFS_PATH will point to /tmp/ipfs_***** and will be
 // cleaned up when the process exits.

-var ipfsd = require('ipfsd-ctl')
+const daemonFactory = require('ipfsd-ctl')
+const local = daemonFactory.localController


Thinking a little bit more about this. Why does the user need to specify a local/remote controller this way?

Wouldn't it be more simpler if there was a setup step:

const DaemonFactory = require('ipfsd-ctl') const df = new DaemonFactory(options)

Where default options is to:

Do local for Node.js

Do remote with default endpoint for Browser.

I thought about doing that at some point, but I settled on not making any assumptions on behalf of the user. I think this is still a valid discussion point, but my reasoning was basically that - let the user choose which controller it wants to use.

The user can still choose. I'm not particularly liking this daemonFactory.localController. API, I feel that the df should be an instance of a DaemonFactory and the user can pick their set up through options.

The one drawback with this approach is that it forces the DF to be stateful rathern than stateless as in the case of simple "static" methods. How about a middle ground? A factory method that will return the correct controller factory? Something like (pseudocode):

class DaemonFactory { static createFactory(options): return ControllerFactory }

where ControllerFactory implements the spawn interface? Note that it doesn't have to be a class, can just as well be a plain old module.exports = function(){}.

Yeah, I like DaemonFactory.create too 👍 (no need for the Factory word again)

daviddias · 2017-12-13T17:42:44Z

README.md


-ipfsd.disposableApi(function (err, ipfs) {
+local.spawn(function (err, ipfsd) {
+  const ipfs = ipfsd.ctl


You missed #176 (comment)

daviddias · 2017-12-13T17:43:17Z

README.md

-ipfsd.disposableApi(function (err, ipfs) {
+local.spawn(function (err, ipfsd) {
+  const ipfs = ipfsd.ctl
+  const node = ipfsd.ctrl


Also missed #176 (comment)

daviddias · 2017-12-13T17:43:54Z

README.md

  ipfs.id(function (err, id) {
    console.log(id)
-    process.exit()
+    node.stopDaemon()


What about clean up? .cleanRepo?

For disposable nodes it happens automatically.

daviddias · 2017-12-14T07:46:30Z

README.md

+  }
+
+  const remote = daemonFactory.remoteController(port || 9999)
+  remote.spawn(function (err, controller) {


Why call it here controller if the other example is ipfsd? Let's ensure to have consistency since for the user, these things will be the same.

daviddias

It is getting there @dryajov ! :) A few more tweeks and then we invite everyone that actively uses this to review it as well. Then it is just about polishing the code.

daviddias · 2017-12-14T07:47:10Z

README.md

+const daemonFactory = require('ipfsd-ctl')
+const server = daemonFactory.server
+
+server.start((err) => {


What are the defaults? Better add here an options object and exemplify that config.

daviddias · 2017-12-14T07:47:31Z

README.md

    console.log(id)
-    process.exit()
+    ipfsCtrl.stopDaemon()


Is there any call for cleanUp??

yes, there is ipfsCtrl.shutdown which will clean up the tmp repo created for the disposable node.

What is the difference between:

.stopDaemon

.shutdown

If I stopDaemon, can I then later clean up?

stopDaemon will kill the process which would call shutdown, that is registered as a process exit handle if the daemon was started as a disposable node. If the node is not disposable, shutdown will not be called and the repo not destroyed (what we want).

I think shutdown would be better if renamed to cleanup.

If I stopDaemon, can I then later clean up?

Yep, that just simply calls rimraf on the repo, so calling after the daemon is stopped is supposed to work.

daviddias · 2017-12-14T07:48:43Z

README.md


-ipfsd.disposableApi(function (err, ipfs) {
-  ipfs.id(function (err, id) {
+local.spawn(function (err, ipfsd) {


What are the options to select:

spawning a disposable node

spawning a node using the ~/.ipfs repo

spawning a node using the ipfs binary available?

daviddias · 2017-12-14T07:49:26Z

README.md

+#### Create factory
+
+- `daemonFactory.localController` - create a local controller
+- `daemonFactory.remoteController([port])` - create a remote controller, usable from browsers


Let's change this to make DaemonFactory a class that takes an options object in the constructor with options to enable the multiple spawning scenarios.

daviddias · 2017-12-14T07:51:10Z

README.md

+
+> Spawn either a js-ipfs or go-ipfs node through `localController` or `remoteController`
+
+`spawn([options], cb)`


s/cb/callback

daviddias · 2017-12-14T07:52:54Z

README.md

+
+ - `cb(err, {ctl: <ipfs-api instance>, ctrl: <Node (ctrl) instance>})` - a callback that receives an object with two members:
+   - `ctl` - an [ipfs-api](https://github.com/ipfs/js-ipfs-api) instance attached to the newly created ipfs node
+   - `ctrl` - an instance of a daemon controller object


- callback is a function with signature function (err, ipfsd) where: - err is an error in case any problem happens with the node spawning - ipfsd is an object that has two properties: - ipfsd.ctl - ... - ipfsd.ctrl - ...

daviddias · 2017-12-14T07:53:36Z

README.md

+
+
+### IPFS Daemon Controller (ctrl)
+


Needs description.

daviddias · 2017-12-14T07:54:47Z

README.md

+
+> Stop the daemon.
+
+- function(Error) callback - function that receives an instance of `Error` on failure


Can a user start -> stop -> start - > stop multiple times? Better add a test for that :)

daviddias

lil more CR

daviddias · 2017-12-14T08:06:45Z

package.json

@@ -50,23 +61,37 @@
  ],
  "license": "MIT",
  "dependencies": {
+    "@ljharb/eslint-config": "^12.2.1",


Is this dep really necessary??

daviddias · 2017-12-14T08:06:58Z

package.json

    "detect-node": "^2.0.3",
    "eslint-config-standard-jsx": "^4.0.2",
    "go-ipfs-dep": "0.4.13",
-    "ipfs-api": "^17.1.3",
+    "guid": "0.0.12",
+    "hapi": "^16.6.2",


Why not use latest hapi?

The latest hapi v17 is a major rewrite for which there is still no updated docs. All the guides/tutorials still reference v16.

daviddias · 2017-12-14T08:07:19Z

package.json

    "subcomandante": "^1.0.5",
    "truthy": "0.0.1"
  },
  "devDependencies": {
    "aegir": "^12.2.0",
    "chai": "^4.1.2",
    "dirty-chai": "^2.0.1",
-    "ipfs": "^0.26.0",
+    "ipfs": "^0.27.0",


Watch out for the ^

daviddias · 2017-12-14T08:08:58Z

src/daemon.js

+    const isJs = truthy(process.env.IPFS_JS)
+
+    this.opts = opts || { isJs: isJs || false }
+    process.env.IPFS_JS = this.opts.isJs


Use env vars as read only. This will mess the spawn of multiple nodes in one go.

hacdias

LGTM. And the tests are passing on Windows! Such a beauty 😃

hacdias

LGTM. And the tests are passing on Windows! Such a beauty 😃

dryajov · 2018-01-10T17:28:25Z

hey @pgte can you do one last review of this PR?

hacdias · 2018-01-11T09:30:47Z

@dryajov is it possible to control an already running IPFS daemon with this? If so, how? 😄

dryajov · 2018-01-11T16:09:21Z

@hacdias it should actually be possible (tho I haven't tested it myself), with df.spawn({disposable: false, start: false, init: false, repoPath: <repoPath of running node>}). If you find that it doesn't work but its something that you need, could you please log it as a separate feature request? (this one is getting really long ;))

dryajov · 2018-01-11T16:16:47Z

@diasdavid is there anything else outstanding (besides @pgte review) to get this released?

vmx

There you go.

vmx · 2018-01-12T14:06:11Z

README.md

+
+- **`go`** - calling `DaemonFactory.create({type: 'go'})` will spawn a `go-ipfs` daemon.
+- **`js`** - calling `DaemonFactory.create({type: 'js'})` will spawn a `js-ipfs` daemon.
+- **`proc`** - calling `DaemonFactory.create({type: 'proc', exec: require('ipfs') })` will spawn an `in process js-ipfs node` using the provided code reference that implements the core IPFS API. Note that, `exec` option to `df.spawn()` is required if `type: 'proc'` is used.


Why do you need to pass in exec when using type: 'proc'? Couldn't it default to require('ipfs')?

That would force users to bring js-ipfs at all times and we learned that makes it hard for users that want to bundle go-ipfs in. Otherwise, it would be good.

Let's call this embedded instead? Seems more consistent with e.g. ipfs-companion. (proc is also a pretty generic name.)

I'm fine either way :) Unless anyone objects, I'll change it. Was trying to find a better name either way in-proc was another option.

vmx · 2018-01-12T14:14:46Z

README.md

+- **`js`** - calling `DaemonFactory.create({type: 'js'})` will spawn a `js-ipfs` daemon.
+- **`proc`** - calling `DaemonFactory.create({type: 'proc', exec: require('ipfs') })` will spawn an `in process js-ipfs node` using the provided code reference that implements the core IPFS API. Note that, `exec` option to `df.spawn()` is required if `type: 'proc'` is used.
+
+#### DaemonFactory endpoint for remote spawning - `const server = DaemonFactory.createServer([options]) `


Nit-pick: Trailing whitespace before the '`'.

vmx · 2018-01-12T14:15:39Z

README.md

+
+#### `ipfsd.apiAddr` (getter)
+
+Get the address (multiaddr) of connected IPFS API. Returns a multiaddr,


Nit-pick: comma instead of dot at the end of sentence.

vmx · 2018-01-12T14:16:42Z

README.md


+#### `ipfsd.repoPath` (getter)
+
+Get the current repo path. Returns string


Nit-pick: Missing dot after sentence end.

vmx · 2018-01-12T14:17:38Z

README.md

+Initialize a repo. 
+
+`initOpts` (optional) is an object with the following properties:
+  - `keysize` (default 2048) - The bit size of the identiy key.


Typo: identiy -> identity

vmx · 2018-01-12T15:09:42Z

test/add-retrive.js

+      expect(this.ipfsd.api).to.have.property('apiPort')
+    })
+
+    it('should be able to store objects', () => {


Doublicated test, it's the same as the one in line 31.

vmx · 2018-01-12T15:12:30Z

test/api.js

+          // Check for props in daemon
+          expect(ipfsd).to.have.property('apiAddr')
+          expect(ipfsd).to.have.property('gatewayAddr')
+          expect(ipfsd.apiAddr).to.not.equal(null)


FYI: Could be written as expect(ipfsd.apiAddr).to.not.be.null(). It's the same for ...to.be.true(). I won't repeat that.

vmx · 2018-01-12T15:14:54Z

test/exec.js

+  // TODO: skip on windows for now
+  // TODO: running under coverage messes up the process hierarchies
+  if (isWindows || process.env['COVERAGE']) {
+    return


Could this be a proper .skip()?

skip doesn't work outside it blocks.

vmx · 2018-01-12T15:20:59Z

test/remote/routes.js

+        expect(res.result.api.apiAddr).to.exist()
+        expect(res.result.api.gatewayAddr).to.exist()
+
+        id = res.result.id


If I understand it correctly, you define the id here for the rest of the tests. Shouldn't the describes be independent and be able to be run in any order? Isn't that something for the setup phase of the tests?

Sure, we could - but we'd probably end up duplicating spawn in a separate test, since id is generated by spawn and the rest of the tests wouldn't work without it.

vmx · 2018-01-12T15:28:02Z

test/start-stop.js

+        })
+      })
+
+      let pid


Same as with id before. Shouldn't the describe()s be independent?

see previous comment.

dryajov · 2018-01-12T15:31:17Z

thanks @vmx fixing :)

dryajov · 2018-01-12T20:39:21Z

awesome stuff @vmx, please let me know if you find anything else.

dryajov · 2018-01-16T14:29:27Z

@diasdavid are we missing anything else from this PR?

daviddias · 2018-01-16T15:03:52Z

It has been merged!!

Thank you so much to everyone that contributed to this PR, either by coding it or reviewing and testing! Lot's of great work here and finally it is simple to spawn both types of IPFS daemons

dryajov · 2018-01-16T15:05:43Z

W00T 👊 🎉 🎈 ❤️ !!!

daviddias · 2018-01-16T15:12:22Z

Time to update all the PRs :)

dryajov · 2018-01-16T15:13:41Z

on it!

ghost assigned dryajov Nov 22, 2017

ghost added the status/in-progress In progress label Nov 22, 2017

dryajov force-pushed the feat/remote-daemon branch from e6fab0e to 7198198 Compare November 22, 2017 22:02

dryajov changed the title ~~WIP: Feat/remote daemon~~ [WIP] Feat/remote daemon Nov 22, 2017

richardschneider suggested changes Nov 23, 2017

View reviewed changes

daviddias reviewed Dec 1, 2017

View reviewed changes

dryajov force-pushed the feat/remote-daemon branch from 5bbe3b5 to b6ad854 Compare December 1, 2017 23:27

daviddias mentioned this pull request Dec 4, 2017

Interop testing ideas and current limitations ipfs/js-ipfs#1030

Closed

dryajov commented Dec 6, 2017

View reviewed changes

dryajov changed the title ~~[WIP] Feat/remote daemon~~ Feat/remote daemon Dec 8, 2017

daviddias requested changes Dec 10, 2017

View reviewed changes

dryajov force-pushed the feat/remote-daemon branch 3 times, most recently from 4537058 to e571a6c Compare December 13, 2017 05:40

daviddias requested changes Dec 13, 2017

View reviewed changes

daviddias mentioned this pull request Dec 14, 2017

[WIP] feat: add ability to start js-ipfs nodes #170

Closed

daviddias reviewed Dec 14, 2017

View reviewed changes

daviddias requested changes Dec 14, 2017

View reviewed changes

dryajov force-pushed the feat/remote-daemon branch from 7df0c81 to 69848c6 Compare December 15, 2017 04:07

daviddias approved these changes Jan 9, 2018

View reviewed changes

richardschneider approved these changes Jan 9, 2018

View reviewed changes

hacdias approved these changes Jan 9, 2018

View reviewed changes

alanshaw approved these changes Jan 11, 2018

View reviewed changes

fix: ipfs-repo is not a devDependency

aa8d72b

dryajov force-pushed the feat/remote-daemon branch from 9dcea69 to aa8d72b Compare January 11, 2018 16:37

daviddias requested a review from vmx January 12, 2018 13:55

daviddias mentioned this pull request Jan 12, 2018

Connect to already running daemon #182

Closed

vmx reviewed Jan 12, 2018

View reviewed changes

dryajov force-pushed the feat/remote-daemon branch from e29690f to 6b4ec15 Compare January 12, 2018 20:37

fix: various fixes from @vmx review

c35f6ce

dryajov force-pushed the feat/remote-daemon branch from 6b4ec15 to c35f6ce Compare January 12, 2018 20:42

daviddias merged commit 1cfbd08 into master Jan 16, 2018

ghost removed the status/in-progress In progress label Jan 16, 2018

daviddias deleted the feat/remote-daemon branch January 16, 2018 14:58

dryajov mentioned this pull request Jan 22, 2018

exec is working as expected, the test needs to be fixed #163

Closed

daviddias mentioned this pull request Feb 15, 2018

Interoperability testing for IPFS ipfs/interop#2

Open

dryajov mentioned this pull request May 8, 2018

Spawn callback can be called multiple times #239

Closed

		@@ -30,23 +30,92 @@ npm install --save ipfsd-ctl

		IPFS daemons are already easy to start and stop, but this module is here to do it from JavaScript itself.


		> Spawn either a js-ipfs or go-ipfs node through `localController` or `remoteController`

		`spawn([options], cb)`


		> Stop the daemon.

		- function(Error) callback - function that receives an instance of `Error` on failure


		#### `ipfsd.apiAddr` (getter)

		Get the address (multiaddr) of connected IPFS API. Returns a multiaddr,


		#### `ipfsd.repoPath` (getter)

		Get the current repo path. Returns string

+                      })
+                    })
+                    let pid

Complete revamp! Spawn js/go daemons locally or remote (from the browser) #176

Complete revamp! Spawn js/go daemons locally or remote (from the browser) #176

Conversation

dryajov commented Nov 22, 2017 • edited by daviddias Loading

In order to merge this PR, we need a working PR for:

The current interface looks like this

What we want

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dryajov commented Nov 23, 2017

dryajov commented Nov 27, 2017 • edited Loading

dryajov commented Nov 29, 2017

Choose a reason for hiding this comment

Powersource commented Dec 1, 2017

dryajov commented Dec 1, 2017

codecov bot commented Dec 6, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

dryajov commented Dec 7, 2017

dryajov commented Dec 7, 2017

dryajov commented Dec 8, 2017

daviddias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dryajov commented Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dryajov Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacdias left a comment

Choose a reason for hiding this comment

hacdias left a comment

Choose a reason for hiding this comment

dryajov commented Jan 10, 2018

dryajov commented Nov 22, 2017 •

edited by daviddias

Loading

dryajov commented Nov 27, 2017 •

edited

Loading

codecov bot commented Dec 6, 2017 •

edited

Loading

dryajov commented Dec 11, 2017 •

edited

Loading

dryajov Dec 14, 2017 •

edited

Loading