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

Implement --profile option in ipfs init #4001

Merged
merged 5 commits into from
Jul 4, 2017
Merged

Implement --profile option in ipfs init #4001

merged 5 commits into from
Jul 4, 2017

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented Jun 21, 2017

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.

@Kubuxu Kubuxu added the status/in-progress In progress label Jun 21, 2017
@magik6k magik6k added status/ready Ready to be worked and removed status/in-progress In progress labels Jun 21, 2017
cmd/ipfs/init.go Outdated
@@ -21,12 +22,49 @@ const (
nBitsForKeypairDefault = 2048
)

// TODO: move this out(where?)
Copy link
Member

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"),
Copy link
Member

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 {
Copy link
Member

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

@magik6k
Copy link
Member Author

magik6k commented Jun 21, 2017

Applied the suggestions

@magik6k
Copy link
Member Author

magik6k commented Jun 22, 2017

This might be solving #1246 too

@magik6k magik6k mentioned this pull request Jun 24, 2017
3 tasks
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]
Copy link
Member

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(...
}

Copy link
Member

@whyrusleeping whyrusleeping left a 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

@whyrusleeping
Copy link
Member

@magik6k do you think it might be better to use the strings option from #4012 here instead of a comma separated list?

@magik6k
Copy link
Member Author

magik6k commented Jun 28, 2017

It would be, I actually checked if that existed when I wrote this. When #4012 gets merged I'll change this to use that.

@whyrusleeping
Copy link
Member

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

@magik6k
Copy link
Member Author

magik6k commented Jul 3, 2017

I agree

@whyrusleeping
Copy link
Member

@magik6k wanna rebase this in hopes of greener tests?

License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@magik6k
Copy link
Member Author

magik6k commented Jul 4, 2017

Rebased. Looks greener too.

@whyrusleeping whyrusleeping merged commit ba9435d into master Jul 4, 2017
@whyrusleeping whyrusleeping deleted the feat/profile branch July 4, 2017 22:16
@whyrusleeping whyrusleeping removed the status/ready Ready to be worked label Jul 4, 2017
@magik6k magik6k restored the feat/profile branch November 27, 2017 03:34
@magik6k magik6k deleted the feat/profile branch November 27, 2017 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants