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
Add support for backup, import by regex, interactive push #8
Conversation
- Listing keys in push is now possible using a regular expression; - Add interactive mode for updating configuration; - Add backup creation prior to import - Template validation can now be skipped for restoring backups
Coverage is still down 0.6% :( |
f4b7af9
to
9f5fef5
Compare
a3405db
to
b8ee247
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.
I'm not really happy with so much unrelated changes in one commit (mainly the first one). This makes the review process very hard and not that efficient. Also being in one pull request means that nothing can be merged even if only one change is not approved.
Also - test should be in the same commit as changes.
@@ -149,3 +150,9 @@ func toString(s interface{}) (string, bool) { | |||
return "", false | |||
} | |||
} | |||
|
|||
func getRegex(key string) (isRegex bool, expression *regexp.Regexp) { |
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 order of the return parameters should be the other way around.
@@ -18,7 +18,9 @@ var fetchCmd = &cobra.Command{ | |||
config := viper.GetStringMap("storage.config") | |||
format := viper.GetString("format") | |||
|
|||
return fetchRun(storage, config, format) | |||
cfg, err := fetchRun(storage, config, format) | |||
fmt.Println(*cfg) |
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 consistency all the printing is done in the *Run function. Can we move this print there?
return err | ||
} | ||
|
||
backup, err := cmd.Flags().GetBool("backup") |
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.
There is a backup functionality (fetch). Having it in two places is obsolete. If you want it here you should expose a way to select filename and basically everything fetch exposes.
cmd/push.go
Outdated
@@ -43,50 +61,154 @@ var pushCmd = &cobra.Command{ | |||
return err | |||
} | |||
|
|||
return pushRun(template, format, key, sourcesList, storage, config, force, !plain) | |||
pushConf := &pushConfig{ |
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 don't like the config struct. Can we make it so it has Run receiver? I guess we should migrate all the Run functions as receivers to a type.
"You will be able to confirm the list before applying it.\n\n") | ||
|
||
return changeset.Refine(func(a interface{}) bool { | ||
b := a.(diff.KVChange) |
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.
You can't assume it is KVChange, different storage can have different way of showing diff - e.g. db storage.
@@ -49,3 +49,8 @@ func (c BuildConfig) Build() ([]byte, error) { | |||
|
|||
return cfgStr, nil | |||
} | |||
|
|||
// BuildNoValidation creates the config based on a non-validated input file. | |||
func (c BuildConfig) BuildNoValidation() ([]byte, 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.
What's that about?
cmd/push.go
Outdated
@@ -61,6 +63,8 @@ var pushCmd = &cobra.Command{ | |||
return err | |||
} | |||
|
|||
plain = usePlain(plain) |
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 function is confusing like that. I think it is better if you make it to be usePretty(plain).
Closing PR - requested changes to be handled in a separate branch/PR. |
push
push
all keys that match it. This can be used together with interactive push