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

feat: new ctl #395

Merged
merged 38 commits into from
Dec 11, 2019
Merged

feat: new ctl #395

merged 38 commits into from
Dec 11, 2019

Conversation

hugomrdias
Copy link
Member

@hugomrdias hugomrdias commented Oct 11, 2019

Adds interface-core common tests setup to ipfsd-ctl so we can dedupe this code from js-ipfs (https://github.com/ipfs/js-ipfs/blob/master/test/utils/interface-common-factory.js#L71) and http-client (https://github.com/ipfs/js-ipfs-http-client/blob/master/test/utils/interface-common-factory.js).

Also adds jsdocs to be easier to transition to this new async setup.

It works basically the same as the createAsync version with one less function call and a new method to create an unmanaged node to be used inside tests you want to stop manually.

PRs to use this in js-ipfs, http-client and interface-core will follow

This PR evolved from adding some helper functions for tests into a full rewrite (sorry about that 😅). While making the supporting PRs i found some problems and inconsistencies that needed to be fix to improve our own and contributors DX.

Problems:

  • Browsers tests skipped cause ctl didn't support proper connectivity to remote nodes
  • We weren't able to tell ctl to use a specific commit of http-client, js-ipfs or cli
  • Options/config between the 3 types of daemons weren't consistent
  • Ctl didn't support remote "in process" daemon
  • IPFS options were handled manually inside ctl, so any change in js-ipfs would require a PR in ctl to support the new options or change to an option

Related issues:

Improvements:

  • better errors
  • DEBUG='ipfsd-ctl:*' everywhere
  • factories for tests with good defaults
  • options are properly merged everywhere
  • safer child_process exit stop()
  • faster stop()
  • IPFS Options are now the same format as https://github.com/ipfs/js-ipfs/blob/master/README.md#ipfs-constructor
  • Ctl can init, start and set config in one cmd (at least with js-ipfs)
  • better docs and jsdocs
  • we can now be sure which http-client, ipfs or go-ipfs is being used
  • utils functions actually work in the browser now
  • works in webworkers now
  • simpler and faster overall
  • disposable node actually clean themselves in the browser
  • better tests
  • ...
  • support electron
  • test in electron

New:

  • new method createController returns a spawned controller
  • createFactory as a second parameter to override options per type

Changes:

  • create change to createFactory
  • createFactory options changed

Old

- `options` - optional object with:
  - `remote` bool - use remote endpoint to spawn the nodes.
  - `port` number - remote endpoint port. Defaults to 43134.
  - `exec` - IPFS executable path. `ipfsd-ctl` will attempt to locate it by default. If you desire to spawn js-ipfs instances in the same process, pass the ref to the module instead (e.g `exec: require('ipfs')`)
  - `type` - the daemon type, see below the options
    - `go` - spawn go-ipfs daemon
    - `js` - spawn js-ipfs daemon
    - `proc` - spawn in-process js-ipfs instance. Needs to be called also with exec. Example: `DaemonFactory.create({type: 'proc', exec: require('ipfs') })`.
  - `IpfsClient` - A custom IPFS API constructor to use instead of the packaged one

New

-   `remote` [boolean] Use remote endpoint to spawn the nodes. Defaults to `true` when not in node.
-   `test` [test=false] - Flag to activate custom config for tests.
-   `endpoint` [endpoint] - Endpoint URL to manage remote Controllers. (Defaults: 'http://localhost:43134').
-   `disposable` [Boolean] A new repo is created and initialized for each invocation, as well as cleaned up automatically once the process exits.
-   `type` [string] The daemon type, see below the options:-   go - spawn go-ipfs daemon
    -   js - spawn js-ipfs daemon
    -   proc - spawn in-process js-ipfs instance
-   `env` [Object] Additional environment variables, passed to executing shell. Only applies for Daemon controllers.
-   `args` [Array] Custom cli args.
-   `ipfsHttp` [Object] Setup IPFS HTTP client to be used by ctl.
    -   `ipfsHttp.ref` [Object] Reference to a IPFS HTTP Client object. (defaults to the local require(`ipfs-http-client`))
    -   `ipfsHttp.path` [string] Path to a IPFS HTTP Client to be required. (defaults to the local require.resolve('ipfs-http-client'))
-   `ipfsApi` [Object] Setup IPFS API to be used by ctl.
    -   `ipfsApi.ref` [Object] Reference to a IPFS API object. (defaults to the local require(`ipfs`))
    -   `ipfsApi.path` [string] Path to a IPFS API implementation to be required. (defaults to the local require.resolve('ipfs'))
-   `ipfsBin` [String] Path to a IPFS exectutable . (defaults to the local 'js-ipfs/src/bin/cli.js')
-   `ipfsOptions` [IpfsOptions] Options for the IPFS instance
  • Previous default ipfs config is only applied when test options equals true
  • defaultAddrs option was removed
  • Spawn options are the same as createFactory

Old

- `options` is an optional object the following properties:
  - `init` bool (default true) or Object - should the node be initialized
  - `initOptions` object - should be of the form `{bits: <size>}`, which sets the desired key size
  - `start` bool (default true) - should the node be started
  - `repoPath` string - the repository path to use for this node, ignored if node is disposable
  - `disposable` bool (default true) - a new repo is created and initialized for each invocation, as well as cleaned up automatically once the process exits
  - `defaultAddrs` bool (default false) - use the daemon default `Swarm` addrs
  - `args` - array of cmd line arguments to be passed to ipfs daemon
  - `config` - ipfs configuration options

NEW
Same as js-ipfs constructor https://github.com/ipfs/js-ipfs#ipfs-constructor

  • ipfsd.killProcess removed not needed anymore
  • ipfsd.getConfig removed call ipfsd.api.config.get instead
  • ipfsd.setConfig removed, call ipfsd.api.config.set instead

TODO:

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Looks good, just some minor things. Lets get this merged and released! 🚀

package.json Outdated Show resolved Hide resolved
src/factory-in-proc.js Outdated Show resolved Hide resolved
src/factory-daemon.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/factory-client.js Outdated Show resolved Hide resolved
src/ipfsd-daemon.js Outdated Show resolved Hide resolved
src/ipfsd-daemon.js Outdated Show resolved Hide resolved
src/ipfsd-daemon.js Outdated Show resolved Hide resolved
@hugomrdias hugomrdias changed the title feat: new ctl [WIP] feat: new ctl Nov 8, 2019
@achingbrain
Copy link
Member

@hugomrdias are you going to polish off the last few nits in this? It'd be good to get it in to unblock a bunch of other stuff.

@alanshaw
Copy link
Member

🙏 pretty please can we have this?

@hacdias
Copy link
Member

hacdias commented Nov 26, 2019

@alanshaw +1

@hugomrdias hugomrdias changed the title [WIP] feat: new ctl feat: new ctl Dec 3, 2019
@hugomrdias
Copy link
Member Author

PR description updated explaining whats new and whats changed.

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

Minor nits, looks good though

README.md Outdated
### Remote endpoint - `const server = IPFSFactory.createServer([options])`

`IPFSFactory.createServer` starts a IPFSFactory endpoint.
### `create([options])`
Copy link
Member

Choose a reason for hiding this comment

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

This might be better named createFactory([options])?

Copy link
Member Author

Choose a reason for hiding this comment

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

damn it was like that i just changed it back yesterday to reduce the diff for the end user.
do you think it's worth it ?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's good to be explicit. The diff is going to be hefty anyway, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the top level api nothing actually changed just the options are different

README.md Outdated Show resolved Hide resolved
*/
start () {
console.warn('Server not implemented in the browser')
return Promise.resolve(this)
Copy link
Member

Choose a reason for hiding this comment

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

Should this (and the other methods in this class) throw instead?

README.md Outdated
@@ -58,11 +58,11 @@ await ipfsd.stop()
// Start a remote disposable node, and get access to the api
// print the node id, and stop the temporary daemon

const IPFSFactory = require('ipfsd-ctl')
const { create, createServer } = require('ipfsd-ctl')
Copy link
Member

Choose a reason for hiding this comment

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

I prefer the previous require style, using create() in code makes me think "what am I creating?" whereas IPFSFactory.create() is obvious.

README.md Outdated

console.log('endpoint has stopped')
```
### `createNodeTests([options])`
Copy link
Member

Choose a reason for hiding this comment

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

Why not just createNode({ test: true })?

Copy link
Member Author

Choose a reason for hiding this comment

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

i can do that sure

README.md Outdated
#### TestsInterface
`nodes` **Array** List of the created controlled nodes.
`node()` **Function** Creates a standalone node, this node will **NOT** be stopped by `teardown()`.
`setup(options)` **Function(**[FactoryOptions](#FactoryOptions)**)** Creates a controlled node.
Copy link
Member

Choose a reason for hiding this comment

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

What does "controlled" mean? Just that they get stopped when teardown is called?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

README.md Outdated
Returns a **[TestsInterface](#TestsInterface)**
#### TestsInterface
`nodes` **Array** List of the created controlled nodes.
`node()` **Function** Creates a standalone node, this node will **NOT** be stopped by `teardown()`.
Copy link
Member

Choose a reason for hiding this comment

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

How about just using the same API but providing an option? setup({ controlled: false })? setup({ managed: false })?

Copy link
Member Author

@hugomrdias hugomrdias Dec 4, 2019

Choose a reason for hiding this comment

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

This needs better wording

node() returns the a ctl node
setup() returns directly an ipfs core api

Should both return the same thing ?

i would rather add a new param than to use an option just to keep FactoryOptions clean. Cause this would be specific to the testInterface.

I also think using the word node here is wrong... We use daemon for process nodes and node for in process etc... but here it's actually a Controller for a IPFS node either process or in-proc.

Would you agree if i change the naming to Controller instead of Node ?

Then ipfsd-ctl would have:

Factory to wrap spawn, version and tmpDir
Controller to wrap the 3 different types of node
plus some methods that return an IPFS Core API.

README.md Outdated

Get the address (multiaddr) of connected IPFS HTTP Gateway. Returns a multiaddr.
### `createServer([options])`
Create an Endpoint Server.
Copy link
Member

Choose a reason for hiding this comment

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

Needs elaboration! What is the server for? Why would you create one? What can it do?

README.md Outdated
- proc - spawn in-process js-ipfs instance
- `env` **[Object]** Additional environment variables, passed to executing shell. Only applies for Daemon controllers.
- `args` **[Array]** Custom cli args.
- `ipfsHttp` **[Object]** Setup IPFS HTTP client to be used by ctl.
Copy link
Member

Choose a reason for hiding this comment

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

If I pass this option are the properties both mandatory? Please can the documentation be clarified?

Copy link
Member Author

@hugomrdias hugomrdias Dec 4, 2019

Choose a reason for hiding this comment

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

nothing is mandatory, but if you really want to make sure you are using a specific version you need to specify both!
ill update the readme with that info !

README.md Outdated
- `ipfsHttp` **[Object]** Setup IPFS HTTP client to be used by ctl.
- `ipfsHttp.ref` **[Object]** Reference to a IPFS HTTP Client object. (defaults to the local require(`ipfs-http-client`))
- `ipfsHttp.path` **[string]** Path to a IPFS HTTP Client to be required. (defaults to the local require.resolve('ipfs-http-client'))
- `ipfsApi` **[Object]** Setup IPFS API to be used by ctl.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced this is the best name for this...especially considering IPFS HTTP Client used to be called ipfs-api and both js-ipfs and js-ipfs-http-client expose the same API.

What about ipfsModule and ipfsHttpClientModule for these?

README.md Outdated
- `ipfsApi.ref` **[Object]** Reference to a IPFS API object. (defaults to the local require(`ipfs`))
- `ipfsApi.path` **[string]** Path to a IPFS API implementation to be required. (defaults to the local require.resolve('ipfs'))
- `ipfsBin` **[string]** Path to a IPFS exectutable . (defaults to the local 'js-ipfs/src/bin/cli.js')
- `ipfsOptions` **[IpfsOptions]** Options for the IPFS instance
Copy link
Member

Choose a reason for hiding this comment

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

Specify when these are applicable? I assume they do not work for HTTP client or go daemon?

Copy link
Member Author

Choose a reason for hiding this comment

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

they work for all 3 types as far as possible exactly like the old version.

  • proc its passed directly
  • daemon it translates as much as possible to args (plus you can manually define and extra args option)
  • client same as daemon cause its just a proxy

this change was to normalise and have just one set possible of options

README.md Outdated

const server = IPFSFactory.createServer({ port: 12345 })
### `createNode([options])`
Creates a node.
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why you'd use createNode and not create?

Copy link
Member Author

Choose a reason for hiding this comment

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

create creates a factory and you get .tmpDir(), .version() and .spawn()

createNode return the node directly basically calls spawn() for you which is what you want 90% of the time.

i didn't made createNode default because there are some use cases that you really need Factory.tmpDir() to create a temp dir for the repo when using a remote node.

src/ipfsd-daemon.js Outdated Show resolved Hide resolved
@hugomrdias
Copy link
Member Author

Moved a few things around to integrate your feedback. Please review again.


Get the version without spawning a daemon
### `createServer([options])`
Create an Endpoint Server. This server is used by a client node to control a remote node. Example: Spawning a go-ipfs node from a browser.
Copy link
Member

Choose a reason for hiding this comment

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

Are there any docs for the API endpoints that are available?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, its mostly to be used by the remote Controller not for public use.

we supported ['--pass some-string'] which is not very common in any process spawning lib so now we only support the common syntax which is ['--pass', 'some-string']
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.

4 participants