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

In proc node config not applied #208

Closed
alanshaw opened this issue Feb 20, 2018 · 7 comments
Closed

In proc node config not applied #208

alanshaw opened this issue Feb 20, 2018 · 7 comments
Labels
exp/expert Having worked on the specific codebase is important kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked

Comments

@alanshaw
Copy link
Member

I'm trying to enable EXPERIMENTAL.dht for my in-proc node:

df.spawn({ config: { EXPERIMENTAL: { dht: true } } }, /* ... */)

The DHT experiment is not enabled because the in process node does not use the config property to configure the node, it expects EXPERIMENTAL to be in the root of the options object:

this.opts.EXPERIMENTAL = defaults({}, opts.EXPERIMENTAL, {

@daviddias daviddias added kind/bug A bug in existing code (including security flaws) status/ready Ready to be worked labels Feb 20, 2018
@dryajov
Copy link
Member

dryajov commented Feb 26, 2018

Hmm, I believe that we can deprecate the use of options in ipfsd-ctl for proc types and always use the config key. That will make it consistent across all daemon types, which currently don't use options at all.

@alanshaw
Copy link
Member Author

Any progress on this? I've encountered it again...

@dryajov
Copy link
Member

dryajov commented May 16, 2018

@alanshaw

TL;DR

We should remove the EXPERIMENTAL option altogether and use the args array to communicate those options. In this case, we'll need to add the mapping for --enable-dht-experiment for in-proc nodes.


The issue is that we already support the --enable-experimental-.... format through the args array for both in proc and deamons.

Here is what we've got currently:

  • the daemon nodes can only enable features through the args array since they are shell commands
  • the in proc nodes are ran within the current process, and hence don't support the args array natively and thus are mapped to the EXPERIMENTAL opts to new IPFS({...})

Here is what I think we should do:

  • leave the daemons as they are (using the args array with the --FLAG flags)
  • remove the EXPERIMENTAL from the in proc nodes altogether and only use args to communicate experimental options along with other possible flags

We can do the cross mapping (i.e. merge the args and the EXPERIMENTAL flags and other options across all implementations), but I think this adds unnecessary complexity and confuses the API.

The main reason I want to use the args array is for backwards compatibility and consistency - this is how all of our nodes are spawned in tests right now.

@jacobheun
Copy link
Contributor

I created #268 to resolve a separate issue I was having with options.Bootstrap not being overridable (which is painful when testing private networks). It does not do what @dryajov is proposing for the experimental args, but it does pass the config to the created IPFS node.

@vasco-santos
Copy link
Member

I would go with @jacobheun solution by now, and we can have @dryajov solution afterward. What do you think? @dryajov @alanshaw

@hugomrdias hugomrdias added exp/expert Having worked on the specific codebase is important P1 High: Likely tackled by core team if no one steps up labels Aug 27, 2018
@momack2 momack2 added this to Ready in ipfs/js-ipfs May 10, 2019
@momack2 momack2 added this to Ready in ipfs/js-waffle May 10, 2019
@daviddias
Copy link
Member

@hugomrdias does this issue still apply?

@hugomrdias
Copy link
Member

not sure but i can guarantee in the next version this will not be a problem, already made tests for it

@hugomrdias hugomrdias mentioned this issue Nov 8, 2019
7 tasks
hugomrdias added a commit that referenced this issue Dec 11, 2019
This is a complete new version of `ipfsd-ctl` dragons ahead!

BREAKING CHANGE:


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:
- #208
- #397
- #374
- #315
- #207
- #217
- and more 
 
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
```md
- `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**
```markdown
-   `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

**Read the README for documention on the new api and options**
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/expert Having worked on the specific codebase is important kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked
Projects
No open projects
Development

No branches or pull requests

6 participants