Skip to content
This repository was archived by the owner on Jun 13, 2023. It is now read-only.

Introduce the keep verb#108

Merged
JulienBalestra merged 4 commits intomasterfrom
JulienBalestra/cli-keep
Aug 21, 2018
Merged

Introduce the keep verb#108
JulienBalestra merged 4 commits intomasterfrom
JulienBalestra/cli-keep

Conversation

@JulienBalestra
Copy link
Copy Markdown
Collaborator

What does this PR do?

Replace the non documented --clean -binaries

Motivation

Add a quicker way to plays with the options.

@JulienBalestra JulienBalestra added the enhancement New feature or request label Aug 15, 2018
@JulienBalestra JulienBalestra added this to the v0.8.0 milestone Aug 15, 2018
Comment thread cmd/cli/cmd.go Outdated
daemonCommand.PersistentFlags().StringP("clean", "c", config.ViperConfig.GetString("clean"), fmt.Sprintf("clean options before %s: %s", setupCommand.Name(), options.GetOptionNames(options.Clean{})))
config.ViperConfig.BindPFlag("clean", daemonCommand.PersistentFlags().Lookup("clean"))

daemonCommand.PersistentFlags().StringP("keep", "k", config.ViperConfig.GetString("keep"), fmt.Sprintf("clean everything but the given options before %s: %s", setupCommand.Name(), options.GetOptionNames(options.Clean{})))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the behaviour of keep if the setup command has never been called? Just a no-op?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's always called by clean.

As the workflow is clean -> setup -> run.

Comment thread pkg/options/options.go Outdated
setAll(opt)
setAllOptionsTo(opt, value)
reflect.ValueOf(opt).Elem().FieldByName("All").SetBool(value)
reflect.ValueOf(opt).Elem().FieldByName("None").SetBool(!value)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of inspecting structs in this way but I do understand why you needed to do it in this case. Even if there was a way to refactor this it should probably be in a different PR, so I'll let it go for now.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's awful. I'm up for any improvement.

Comment thread pkg/options/clean.go
// The clean string is lowercase clean options comma separated like: etcd,binaries ...
func NewCleanOptions(cleanString string) *Clean {
return newOptions(cleanString, &Clean{}).(*Clean)
// keepString takes precedence over the clean one
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add something like keepString takes precedence over the clean one in the docs for the keep command?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed

Comment thread pkg/options/options.go Outdated
}

func newOptions(stringOptions string, opt interface{}) interface{} {
func newOptions(stringOptions string, value bool, opt interface{}) interface{} {
Copy link
Copy Markdown
Contributor

@devonboyer devonboyer Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename the local variable value to drain/clean maybe?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of something like enabled?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devonboyer I just pushed the changes

@JulienBalestra JulienBalestra merged commit 72c3077 into master Aug 21, 2018
@JulienBalestra JulienBalestra deleted the JulienBalestra/cli-keep branch August 21, 2018 19:04
@JulienBalestra JulienBalestra mentioned this pull request Aug 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants