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

feat: add keysize through an option to spawn #203

Merged
merged 15 commits into from
Feb 21, 2018
Merged

feat: add keysize through an option to spawn #203

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2595fc4
feat: add keysize through env or as an option to spawn
dryajov Feb 16, 2018
718e0ce
fix: lint
dryajov Feb 16, 2018
c9c25da
fix: don't pass on arguments from node executable
vmx Feb 16, 2018
c3fca62
use initOpts istead of reusing init
dryajov Feb 16, 2018
55142c5
feat: replace config on init instead of setting it once for each option
dryajov Feb 17, 2018
17d578e
use cross-env to set env vars and not break windows
dryajov Feb 17, 2018
f5f400e
test: skip multiple start on windows
dryajov Feb 17, 2018
ee74427
test: skip spawn from a initialized repo on windows
dryajov Feb 17, 2018
b167a3c
feat: small changes from review
dryajov Feb 20, 2018
ac70cda
fix: setting keysize on each tests
dryajov Feb 20, 2018
6c7a028
fix: several changes from review
dryajov Feb 21, 2018
2ca9aa1
feat: proc nodes should run whit bits
dryajov Feb 21, 2018
1351dde
fix: dont forget about bits in add retrieve
dryajov Feb 21, 2018
fb30e0e
chore: update ipfs-api
dryajov Feb 21, 2018
174e9dc
timeouts
dryajov Feb 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .aegir.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
const createServer = require('./src').createServer

const server = createServer() // using defaults

module.exports = {
karma: {
files: [{
Expand Down
3 changes: 2 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,8 @@ Install one or both of the following modules:
Spawn the daemon

- `options` is an optional object the following properties:
- `init` bool (default true) - should the node be initialized
- `init` bool (default true) or Object - should the node be initialized
- if `init` is an Object, it is expected to be of the form `{keySize: <bits>}`, this will set the `keysize` to initialize the daemon with
Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of conditional types in a parameter. Either it's a Boolean or it's a Object. Rather, we should add a new option. If we're only adding keySize, why not make keySize a key of the options object?

Copy link
Member

Choose a reason for hiding this comment

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

I also think we call the option bits here, OR, change the option in go-ipfs as well. It's bits there currently, but keySize is probably a more descriptive name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @victorbjelkholm, I thought about that too and you're right, I'll add another options initOpts and leave init as a bool.

For the second comment, keysize is used because historically that is what ipfsd-ctl used and it's the same name we use when passing that flag to init, and yes I think keysize is more descriptive.

Copy link
Member

@daviddias daviddias Feb 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

read: #188 (comment)

I'm fine with init being boolean or object, but I'm fine with initOptions (not initOpts) too.

Copy link
Member

Choose a reason for hiding this comment

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

README needs to be updated to include initOptions

- `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
Expand Down
3 changes: 2 additions & 1 deletion src/factory-daemon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ class FactoryDaemon {
* Spawn an IPFS node, either js-ipfs or go-ipfs
*
* Options are:
* - `init` bool - should the node be initialized
* - `init` {bool|Object} - should the node be initialized
* - if `init` is an Object, it is expected to be of the form `{keySize: <bits>}`
* - `start` bool - should the node be started
* - `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
Expand Down
3 changes: 2 additions & 1 deletion src/factory-in-proc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ class FactoryInProc {
* Spawn JSIPFS instances
*
* Options are:
* - `init` bool - should the node be initialized
* - `init` {bool|Object} - should the node be initialized
* - if `init` is an Object, it is expected to be of the form `{keySize: <bits>}`
* - `start` bool - should the node be started
* - `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
Expand Down
18 changes: 17 additions & 1 deletion src/ipfsd-daemon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,14 @@ class Daemon {
this._gatewayAddr = null
this._started = false
this.api = null
this.keySize = null

this.keySize = process.env.IPFS_KEYSIZE

// option takes precedence over env variable
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is right. First it should use the default value. Second it should use the passed in value and lastly, if there is a environment variable, use that.

This way we can override application configs when deploying etc.

Copy link
Member

Choose a reason for hiding this comment

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

👍 for what @victorbjelkholm said.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait, what @victorbjelkholm said reads as - default take precedence over env, which takes precedence over option?

Seems backwards, unless I'm reading it wrong. In any case, this is what I'm doing now - option takes precedence over ENV variable, which takes precedence over default. I think that is the canonical way of doing it and allows passing defaults with the ENV variable, but override them on a case by case basis in tests cases and elsewhere?

Copy link
Member

Choose a reason for hiding this comment

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

Default -> Options trumps Default -> Env trumps Options and Default

Copy link
Member

@daviddias daviddias Feb 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@dryajov I believe you are using precedence as "it comes before === less value" but the english definiton says "precedence: the condition of being considered more important than someone or something else; priority in importance, order, or rank." and so, if default took precedence over env, default values would always win.

Copy link
Member Author

Choose a reason for hiding this comment

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

option takes precedence over ENV variable, which takes precedence over default.

Which uses precedence in the english definition of the term.

I think the contention is whether ENV take precedence over option or vice versa. The case I'm making is that an ENV variable will define a global that should be overridable through an option when calling spawn. This allows to override the default globally and locally on a case by case basis.

Copy link
Member Author

@dryajov dryajov Feb 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

So in my case it is -

Default -> Env trumps Default -> Options trumps Default and Env

But I'm not dead set on it, but it feels more natural to me.

It also follows well established scoping rules - IMO.

Updated with last line

Copy link
Member

Choose a reason for hiding this comment

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

@dryajov use #203 (comment), which is pretty much how every system works.

I wonder which system are you familiar with that uses it the other way around.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any programming language with correct scoping rules :)

if (typeof this.opts.init === 'object') {
this.keySize = this.opts.init.keySize
}

if (this.opts.env) {
Object.assign(this.env, this.opts.env)
Expand Down Expand Up @@ -128,11 +136,19 @@ class Daemon {
this.path = initOpts.directory
}

const args = ['init', '-b', initOpts.keysize || 2048]
const keySize = initOpts.keysize ? initOpts.keysize : this.keySize
const args = ['init']
// do not just set a default keysize,
// in case we decide to change it at
// the daemon level in the future
Copy link
Member

Choose a reason for hiding this comment

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

This sentence doesn't read right. Do you mean "Skip if default daemon keysize"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, ipfs daemons have a default keysize when using ipfs init - if we pass a value with ipfs init -b <keysize>, we're overriding that default (we're currently doing that in latest master). I don't think that is correct and we should rely on the default used by the daemon internally.

if (keySize) {
args.concat(['-b', keySize])
}
if (initOpts.pass) {
args.push('--pass')
args.push('"' + initOpts.pass + '"')
}
log(`initializing with keysize: ${keySize}`)
run(this, args, { env: this.env }, (err, result) => {
if (err) {
return callback(err)
Expand Down
17 changes: 16 additions & 1 deletion src/ipfsd-in-proc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class Node {
this._started = false
this.initialized = false
this.api = null
this.keySize = null

this.opts.EXPERIMENTAL = defaults({}, opts.EXPERIMENTAL, {
pubsub: false,
Expand All @@ -55,6 +56,14 @@ class Node {
throw new Error('Unkown argument ' + arg)
}
})

// option takes precedence over env variable
if (typeof this.opts.init === 'object') {
this.keySize = this.opts.init.keySize
} else if (process.env.IPFS_KEYSIZE) {
this.keySize = process.env.IPFS_KEYSIZE
}

this.exec = new IPFS({
repo: this.repo,
init: false,
Expand Down Expand Up @@ -126,7 +135,13 @@ class Node {
initOpts = {}
}

initOpts.bits = initOpts.keysize || 2048
const keySize = initOpts.keysize ? initOpts.keysize : this.keySize
// do not just set a default keysize,
// in case we decide to change it at
// the daemon level in the future
if (keySize) {
initOpts.bits = keySize
}
this.exec.init(initOpts, (err) => {
if (err) {
return callback(err)
Expand Down