-
-
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
Implement --profile option in ipfs init #4001
Conversation
cmd/ipfs/init.go
Outdated
@@ -21,12 +22,49 @@ const ( | |||
nBitsForKeypairDefault = 2048 | |||
) | |||
|
|||
// TODO: move this out(where?) |
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.
How about repo/config/profiles.go
?
cmd/ipfs/init.go
Outdated
@@ -40,6 +78,7 @@ environment variable: | |||
Options: []cmds.Option{ | |||
cmds.IntOption("bits", "b", "Number of bits to use in the generated RSA private key.").Default(nBitsForKeypairDefault), | |||
cmds.BoolOption("empty-repo", "e", "Don't add and pin help files to the local storage.").Default(false), | |||
cmds.StringOption("profile", "p", "Apply profile settings to config"), |
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.
It's non-obvious that you can apply multiple comma-separated profiles.
cmd/ipfs/init.go
Outdated
// TODO: move this out(where?) | ||
// ConfigProfiles is a map holding configuration transformers | ||
var ConfigProfiles = map[string]func(*config.Config) error{ | ||
"server": func(c *config.Config) error { |
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.
for server, you'll also want to disable mdns
Applied the suggestions |
This might be solving #1246 too |
cmd/ipfs/init.go
Outdated
@@ -132,6 +152,17 @@ func doInit(out io.Writer, repoRoot string, empty bool, nBitsForKeypair int, con | |||
} | |||
} | |||
|
|||
for _, profile := range confProfiles { | |||
transformer := config.ConfigProfiles[profile] |
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.
transformer, ok := ....
if !ok {
return fmt.Errorf(...
}
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.
one nitpick, then LGTM
It would be, I actually checked if that existed when I wrote this. When #4012 gets merged I'll change this to use that. |
Hrm... we have a weird dependency graph here. This currently feels like its blocked on #4012 which is from an external contributor, so we don't know when it will be completed. Perhaps we should move forward with this as is for now (with the commas) and add support for the 'strings' option later on once that is merged? should be pretty trivial to maintain backwards compatibility there |
I agree |
@magik6k wanna rebase this in hopes of greener tests? |
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
Rebased. Looks greener too. |
Closes #3987
This implements
ipfs init --profile ...
which allows to apply profiles to default config. The server profile does what was requested in #3987.Profiles can be stacked, this may be very useful when the configurable datastore PR is merged and there are more datastores.