-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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: add ability to use existing config during init #6489
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start. We can do merging in a followup.
cmd/ipfs/init.go
Outdated
profile, _ := req.Options[profileOptionName].(string) | ||
|
||
if cfgLocation != "" { | ||
if profile != "" { | ||
return errInitConfigArgs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to reject this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not.
I was thinking we needed to merge profiles rather than use the config transformers. I'll update this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so this works now.
set IPFS_PATH=Z:\ipfs-dev-tmp1
ipfs init
set IPFS_PATH=Z:\ipfs-dev-tmp2
ipfs daemon --init --init-config-file=Z:\ipfs-dev-tmp1\config --init-profile=badgerds,randomports
diff Z:\ipfs-dev-tmp1 Z:\ipfs-dev-tmp2
Only in Z:\ipfs-dev-tmp2: badgerds
Only in Z:\ipfs-dev-tmp1: blocks
diff "Z:\\ipfs-dev-tmp1/config" "Z:\\ipfs-dev-tmp2/config"
11,34c11,18
< "mounts": [
< {
< "child": {
< "path": "blocks",
< "shardFunc": "/repo/flatfs/shard/v1/next-to-last/2",
< "sync": true,
< "type": "flatfs"
< },
< "mountpoint": "/blocks",
< "prefix": "flatfs.datastore",
< "type": "measure"
< },
< {
< "child": {
< "compression": "none",
< "path": "datastore",
< "type": "levelds"
< },
< "mountpoint": "/",
< "prefix": "leveldb.datastore",
< "type": "measure"
< }
< ],
< "type": "mount"
---
> "child": {
> "path": "badgerds",
> "syncWrites": true,
> "truncate": true,
> "type": "badgerds"
> },
> "prefix": "badger.datastore",
> "type": "measure"
41,42c25,26
< "/ip4/0.0.0.0/tcp/4001",
< "/ip6/::/tcp/4001"
---
> "/ip4/0.0.0.0/tcp/16936",
> "/ip6/::/tcp/16936"
Only in Z:\ipfs-dev-tmp1: datastore
diff "Z:\\ipfs-dev-tmp1/datastore_spec" "Z:\\ipfs-dev-tmp2/datastore_spec"
1c1
< {"mounts":[{"mountpoint":"/blocks","path":"blocks","shardFunc":"/repo/flatfs/shard/v1/next-to-last/2","type":"flatfs"},{"mountpoint":"/","path":"datastore","type":"levelds"}],"type":"mount"}
\ No newline at end of file
---
> {"path":"badgerds","type":"badgerds"}
\ No newline at end of file
Common subdirectories: Z:\ipfs-dev-tmp1/keystore and Z:\ipfs-dev-tmp2/keystore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say allow both (but we can stick to transformers for now). That is:
- Take the initial config, if supplied.
- Merge it with the default config (later).
- Apply transformers (if specified).
73615a8
to
69893e3
Compare
Rewrote the first commit to be more accurate - cmds.StringOption(configOptionName, "Use supplied config instead of generating one"),
+ cmds.StringOption(configOptionName, "Use supplied config as a base instead of the default"), Edit: |
69893e3
to
4bb2df4
Compare
cmd/ipfs/init.go
Outdated
var profiles []string | ||
if profile != "" { | ||
profiles = strings.Split(profile, ",") | ||
if cfgLocation != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, it looks like we currently accept one argument for the "initial config". We should:
- Make sure we aren't passed both.
- Consider deprecating that?
Or maybe we don't need this flag on init (we could only support passing the config as an argument on init?). @hugomrdias, what's the state of javascript here?
cmd/ipfs/init.go
Outdated
return err | ||
} | ||
|
||
return doInit(os.Stdout, cctx.ConfigRoot, false, nBitsForKeypairDefault, profiles, conf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the doInit
call below? This one drops the empty
flag and passes the wrong nBitsForKeypair
option (although I guess that doesn't do anything right now).
915e376
to
7b3e818
Compare
Status: Blocked on cc @hugomrdias? |
What does |
@lanzafame at the moment, it initializes with an existing config. |
Whether this was intended or not, I have to be pedantic here since it's relevant.
Calling that results in this >set IPFS_PATH=Z:\itmp-1
>ipfs init
initializing IPFS node at Z:\itmp-1
...
>set IPFS_PATH=Z:\itmp-2
>ipfs init --config=Z:\itmp-1\config
initializing IPFS node at Z:\itmp-1\config
Error: unexpected error while checking writeablility of repo root: open Z:\itmp-1\config\test: The system cannot find the path specified. It's also a bit of a hasty blunder on my part to have forgotten the existing If we can, I'm not opposed to using Explicitly pointing out that |
Oh dear. Yeah, we need to make sure js/go agree here or it's going to be really confusing. |
in js:
making ipfs [cmd] --config point to an out-of-repo config will make so maybe,
makes sense ? On js side i'll code whatever we decide here no blockers there. |
7b3e818
to
8c9fab9
Compare
Went ahead with the change. |
@Stebalien can we unblock this ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, code LGTM (sorry for the delay).
@djdv could you write a quick sharness test for this?
0755391
to
c85a98f
Compare
c85a98f
to
21b8aba
Compare
@djdv anything i can do to help with this ? |
@hugomrdias |
56746d0
to
1a89933
Compare
f12f59d
to
6996faa
Compare
6996faa
to
e7e7000
Compare
This PR aligns `ipfs init` and `ipfs daemon` with go-ipfs. ipfs/kubo#6489 `ipfs init` changed to accept a file path as an argument `ipfs daemon` changed to support `--init` and `--init-config` options. Now we can do `ipfs daemon --init --init-config /path/to/custom-config` refs: ipfs/js-ipfsd-ctl#303
This PR aligns `ipfs init` and `ipfs daemon` with go-ipfs. ipfs/kubo#6489 `ipfs init` changed to accept a file path as an argument `ipfs daemon` changed to support `--init` and `--init-config` options. Now we can do `ipfs daemon --init --init-config /path/to/custom-config` refs: ipfs/js-ipfsd-ctl#303
This PR aligns `ipfs init` and `ipfs daemon` with go-ipfs. ipfs/kubo#6489 `ipfs init` changed to accept a file path as an argument `ipfs daemon` changed to support `--init` and `--init-config` options. Now we can do `ipfs daemon --init --init-config /path/to/custom-config` refs: ipfs/js-ipfsd-ctl#303
This PR aligns `ipfs init` and `ipfs daemon` with go-ipfs. ipfs/kubo#6489 `ipfs init` changed to accept a file path as an argument `ipfs daemon` changed to support `--init` and `--init-config` options. Now we can do `ipfs daemon --init --init-config /path/to/custom-config` refs: ipfs/js-ipfsd-ctl#303
This PR aligns `ipfs init` and `ipfs daemon` with go-ipfs. ipfs/kubo#6489 `ipfs init` changed to accept a file path as an argument `ipfs daemon` changed to support `--init` and `--init-config` options. Now we can do `ipfs daemon --init --init-config /path/to/custom-config` refs: ipfs/js-ipfsd-ctl#303
Related to:
#6262
This patch adds
ipfs daemon --init --init-config-file=${PATH}
and
ipfs init --config-file=${PATH}
allowing users to specify existing configuration files instead of (re)generating them.
Does not deal with merging of existing configs and/or profiles (yet?).