Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat: add support to pass config in the init cmd #1662

Merged
merged 2 commits into from
Oct 31, 2018
Merged

Conversation

hugomrdias
Copy link
Member

@hugomrdias hugomrdias commented Oct 24, 2018

This PR adds support to pass node config in the init cmd. The main purpose for this is to improve the speed of ipfsd-ctl and all the tests that use ctl.

example: ipfs init '{"Addresses":{"Swarm":["/ip4/0.0.0.0/tcp/4003"]}}'
This merges the input config with the default config, just like the node api already does.

Check ipfsd-ctl PR related to this one for speed improvements ipfs/js-ipfsd-ctl#303

Right now we need to spawn 4 child_processes to start a node in ctl

  • ipfs init
  • ipfs config show
  • ipfs config replace
  • ipfs daemon

With this PR we would only need 2

  • ipfs init
  • ipfs daemon

And with further changes we could do ipfs daemon --init --config "{}"

Right now i think we have a couple of problems with this

  • go ipfs init needs the config to be in a file and needs it to be a full config, no merging
  • other config changing cmds only do replacing no merging

Questions:

  • How can we get the Go default config to manually do the merging?
  • Can we get Go to do merging?
  • Can we do this change in js without Go supporting it?
  • Shouldn't we have a ipfs config merge ?

Go version works like this https://docs.ipfs.io/reference/api/cli/#ipfs-init

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

👏🏽👏🏽👏🏽👏🏽 great catch!!

@daviddias
Copy link
Member

@alanshaw can we get this one in the upcoming release?

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

👍

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.

Awesome @hugomrdias - just one question, everything else LGTM

@@ -4,6 +4,7 @@ const peerId = require('peer-id')
const waterfall = require('async/waterfall')
const parallel = require('async/parallel')
const promisify = require('promisify-es6')
const extend = require('deep-extend')
Copy link
Member

Choose a reason for hiding this comment

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

Are we able to use @nodeutils/defaults-deep here? It's a missing dependency that we decided not to add here #1663 in favour of @nodeutils/defaults-deep.

@hugomrdias
Copy link
Member Author

@alanshaw swap deep-extend with defaults-deep with last commit

@alanshaw
Copy link
Member

Just waiting for CI

@daviddias
Copy link
Member

image

@daviddias daviddias merged commit 588891c into master Oct 31, 2018
@daviddias daviddias deleted the feat/init-config branch October 31, 2018 08:04
@ghost ghost removed the status/in-progress In progress label Oct 31, 2018
@hugomrdias
Copy link
Member Author

🎉🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants