-
-
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
config: command to apply profile after init #4195
Conversation
It would be nice to print out what changes are being made |
865f4fb
to
128e6ba
Compare
core/commands/config.go
Outdated
Run: func(req cmds.Request, res cmds.Response) { | ||
args := req.Arguments() | ||
|
||
profile, ok := config.Profiles[args[0]] |
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.
instead of binding it to args
and just using the [0]
on it, why not do:
profilename := req.Arguments()[0]
repo/config/profile.go
Outdated
"/ip4/203.0.113.0/ipcidr/24", | ||
"/ip4/240.0.0.0/ipcidr/4", | ||
} | ||
|
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.
Not really related to this PR, but i was just thinking that we should probably add LAN addresses to the NoAnnounce
field for the server 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.
Done
a017c41
to
48f1069
Compare
core/commands/config.go
Outdated
|
||
profile, ok := config.Profiles[req.Arguments()[0]] | ||
if !ok { | ||
res.SetError(fmt.Errorf("%s in not a profile", req.Arguments()[0]), cmds.ErrNormal) |
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.
s/in/is/
core/commands/config.go
Outdated
} | ||
|
||
func transformConfig(configRoot string, transformer config.Transformer) error { | ||
r, err := fsrepo.Open(configRoot) |
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.
if the daemon is running while the user runs this they might run into issues. You cant open the repo again
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 best to pass in the repo from the IpfsNode
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.
That's what other commands here do. Looking at repo.Open it should be safe:
func Open(repoPath string) (repo.Repo, error) {
fn := func() (repo.Repo, error) {
return open(repoPath)
}
return onlyOne.Open(repoPath, fn)
}
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.
oh, i see. good point.
eeaaceb
to
ca5e92f
Compare
ca5e92f
to
a956439
Compare
docs/config.md
Outdated
@@ -267,3 +268,17 @@ Disables the p2p-circuit relay transport. | |||
- `EnableRelayHop` | |||
Enables HOP relay for the node. If this is enabled, the node will act as | |||
an intermediate (Hop Relay) node in relay circuits for connected peers. | |||
|
|||
## Profiles |
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.
Hrm... i'm not sure this belongs here. Maybe put it at the top of the document? to distinguish it from just looking like another config field
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.
Moved it to the header, makes a bit more sense now
a956439
to
c875c01
Compare
@whyrusleeping Is there anything else to do here? Getting this in would make #4154 a bit easier for me to test. |
repo/config/profile.go
Outdated
c.Discovery.MDNS.Enabled = false | ||
return nil | ||
}, | ||
Unapply: func(c *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.
Maybe call it Revert
instead of Unapply
?
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.
Done
One small comment, then LGTM |
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.
See individual comments.
Also could use some some ore comprehensive tests.
core/commands/config.go
Outdated
var configProfileRevertCmd = &cmds.Command{ | ||
Helptext: cmds.HelpText{ | ||
Tagline: "Revert profile changes.", | ||
ShortDescription: `Reverts profile-related changes to the 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.
This should be Reverts profile-related changes to the default config.
.
docs/config.md
Outdated
Reduces external interference, useful for running ipfs in test environments. | ||
Note that with these settings node won't be able to talk to the rest of the | ||
network without manual bootstrap. | ||
|
||
## Table of Contents |
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.
@whyrusleeping should we document the badgerds
profile here but label it as experimental?
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.
Its somewhat out of scope, but probably a good idea.
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.
Out of scope?
I meant to add under the new Profiles section in case that wasn't clear.
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.
@kevina ah, gotcha. Yeah, lets get that in here.
I also like we should create backup files automatically. We could call them: @whyrusleeping others, thoughs? |
@kevina I do like the idea of making backups automatically. That will require some changes to the repo abstraction I think. |
248e971
to
81579d6
Compare
|
License: MIT Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT Signed-off-by: Łukasz Magiera <[email protected]>
917f232
to
bebdd2b
Compare
Backups done |
repo/mock.go
Outdated
@@ -28,6 +28,10 @@ func (m *Mock) SetConfig(updated *config.Config) error { | |||
return nil | |||
} | |||
|
|||
func (m *Mock) BackupConfig(prefix string) (string, error) { | |||
return "config-" + prefix + "-backup", nil |
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.
maybe this can just return errTODO
like the other methods? If we arent actually going to back up the config, we probably should bother to pretend with the name
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.
done
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.
The backups look pretty cleanly done, I just have one comment on the mock repo.
License: MIT Signed-off-by: Łukasz Magiera <[email protected]>
bebdd2b
to
0ff9b24
Compare
docs/config.md
Outdated
If you apply this profile after `ipfs init`, you will need to convert your | ||
datastore to the new configuration. You can do this using [ipfs-ds-convert](https://github.com/ipfs/ipfs-ds-convert) | ||
|
||
WARNING: badger datastore is experimantal. Make sure to backup your data |
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.
badger datastore is experimental
docs/config.md
Outdated
- `server` | ||
|
||
Recommended for nodes with public IPv4 address, disables host and content | ||
discovery in local networks. |
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.
Or those running on VPSs.
core/commands/config.go
Outdated
|
||
Subcommands: map[string]*cmds.Command{ | ||
"apply": configProfileApplyCmd, | ||
"revert": configProfileRevertCmd, |
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.
Actually reverting is impossible (for us). We're not reverting, we're applying the inverse of the profile. Do we really need to provide this feature?
We could, instead, provide inverse profiles: local-discovery and flatfs-ds.
I'm worried that a user will think that profiles act like drop-ins (that's how a lot of unix daemons work) and will believe that revert will revert the apply.
Alternatively, we could just do this with layered drop-ins (but that's more complex).
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 agree with @Stebalien this is not really a revert so we perhaps we should name it something else, not sure what, "unapply" comes to mind.
The idea of instead providing inverse profiles is also a good idea.
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 think I like the idea of inverse profiles better, I'll see how that looks
repo/config/profile.go
Outdated
"/ip4/240.0.0.0/ipcidr/4", | ||
} | ||
|
||
c.Addresses.NoAnnounce = append(c.Addresses.NoAnnounce, defaultServerFilters...) |
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.
We should probably try merging these instead of just appending.
repo/config/profile.go
Outdated
return nil | ||
}, | ||
Revert: func(c *Config) error { | ||
c.Addresses.NoAnnounce = []string{} |
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.
If we do keep the revert command, we should only remove the ones we've added.
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 would use more helpful names for the backup. The name config-profile-444132629
is not very descriptive. This also makes it hard to find the right config backup to undo the change.
I made suggestions for improving on this. However, I am not set on this, so I can change my review if no one else agrees with me.
If I was writing this I would also use a timestamp rather then TempFile so that if there are multiple backups it is clear what the latest one is. Another option would be to just overright the old backup, that is what most other programs would do.
repo/config/profile.go
Outdated
c.Discovery.MDNS.Enabled = false | ||
return nil | ||
// Transformer is a function which takes configuration and applies some filter to it | ||
type Transformer func(c *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.
I would make this a struct something like
type Transformer struct {
Name string
Apply func(c *Config) error
}
core/commands/config.go
Outdated
return err | ||
} | ||
|
||
_, err = r.BackupConfig("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.
After making config.Transformer
I would make this
_, err = r.BackupConfig(transformer.Name + "-")
To provide more helpful backup names.
License: MIT Signed-off-by: Łukasz Magiera <[email protected]>
Refactored revert into inverse profiles, cc @Stebalien |
test/sharness/t0021-config.sh
Outdated
ipfs config profile apply '${inverse_profile}' | ||
' | ||
|
||
test_expect_success "config is back to previous state after ${profile} revert" ' |
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.
This test is a bit misnamed now.
c19cc7f
to
0546f1f
Compare
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.
Some more nitpicks on how backup are handled.
Otherwise looks good.
docs/config.md
Outdated
#### Profiles | ||
Configuration profiles allow to tweak configuration quickly. Profiles can be | ||
applied with `--profile` flag to `ipfs init` or with `ipfs config profile apply` | ||
command. |
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 would mention that a backup is created and what it is called.
core/commands/config.go
Outdated
}, | ||
} | ||
|
||
func transformConfig(configRoot string, backupName string, transformer config.Transformer) 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.
I would call backupName
configName
.
core/commands/config.go
Outdated
return err | ||
} | ||
|
||
_, err = r.BackupConfig(backupName + "-") |
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.
Nit. I would make this "pre-" + configName + "-"
to make it clear that the backup before the config was applied.
License: MIT Signed-off-by: Łukasz Magiera <[email protected]>
0546f1f
to
ac26cf1
Compare
Finally getting this one in, Thanks @magik6k ! |
TODO: