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

Fixes #25307 - Add ability to disable defaults #293

Merged
merged 4 commits into from Nov 2, 2018

Conversation

Projects
None yet
4 participants
@mbacovsky
Copy link
Member

mbacovsky commented Oct 26, 2018

Presence of Defaults can be set in cli_config.yml with
:use_defaults: true/false or on CLI with --use-defaults
--no-use-defaults respectively. both ways can be combined.
The value is propagated in the context as :use_defaults.
The defaults are enabled by default.

Hammer settings class was chenged so that it can be the single
source of default values which has three benefits

  • we don't have to fallback to default with every use of the value
  • we can keep the values commented out in config which makes it easier
    to changelater
  • there is single place to look for the default setting
@tstrachota
Copy link
Member

tstrachota left a comment

Looks about right. I added some refactoring suggestions for better code readability.

@@ -59,9 +64,16 @@ def self.path_history
@path_history
end

def self.default_values

This comment has been minimized.

@tstrachota

tstrachota Oct 26, 2018

Member

How about renaming it to default_settings ?
It's probably just because of this PR's context, but the name associated me different functionality.

end

HammerCLI::MainCommand.subcommand "defaults", _("Defaults management"), HammerCLI::DefaultsCommand
if HammerCLI::Settings.get(:use_defaults)

This comment has been minimized.

@tstrachota

tstrachota Oct 26, 2018

Member

I think we should keep the command in. I find it confusing that a command would disappear when you pass --no-defaults

This comment has been minimized.

@mbacovsky

mbacovsky Oct 26, 2018

Author Member

@tstrachota Removing commands was easy way to prevent the defaults.yaml from rewriting when user stores new value with defaults disabled. I can make it fail on attempt to store the defaults. Would that be better?

This comment has been minimized.

@ofedoren

ofedoren Oct 26, 2018

Member

I'd vote for fail on attempt and showing user a some kind of warning?

This comment has been minimized.

@tstrachota

tstrachota Nov 1, 2018

Member

IMHO it's totally fine to save and list defaults even --no-use-defaults is passed. The option should just deactivate using the defaults in requests.

return @defaults if @defaults
settings = HammerCLI::Settings
default_values = settings.get(:use_defaults) ? settings.get(:defaults) : {}
@defaults = Defaults.new(default_values)

This comment has been minimized.

@tstrachota

tstrachota Oct 26, 2018

Member

Maybe it's just me, but this seems difficult to read.
How about:

@defaults ||= Defaults.new(default_values_from_settings)

private
def default_values_from_settings
  HammerCLI::Settings.get(:use_defaults) ? HammerCLI::Settings.get(:defaults) : {}
end

This comment has been minimized.

@mbacovsky

mbacovsky Oct 26, 2018

Author Member

@tstrachota it's a way better! You were awarded by the rubocop of the day trophy. 🤖

@@ -92,6 +94,8 @@ HammerCLI::Settings.load({
:ssl_with_basic_auth => preparser.ssl_with_basic_auth?
}})

HammerCLI::Settings.load({:use_defaults => preparser.use_defaults?}) unless preparser.use_defaults?.nil?

This comment has been minimized.

@ofedoren

ofedoren Oct 26, 2018

Member

Isn't it a duplication of https://github.com/theforeman/hammer-cli/pull/293/files#diff-aca6a835822ab9b99782be0a5f01f8ddR87 ? If it's a shorcut for Settings.get(:_params, :use_defaults), why keep it under _params then?

This comment has been minimized.

@mbacovsky

mbacovsky Oct 26, 2018

Author Member

@ofedoren Exactly. I had a reason to have it there but it was bad idea indeed. Removed.

Fixes #25307 - Add ability to disable defaults
Processing of defaults can be altered in cli_config.yml with
:use_defaults: true/false or on CLI with --use-defaults
--no-use-defaults respectively. both ways can be combined.
The value is propagated in the context as :use_defaults.
The defaults are enabled by default.

Hammer settings class was changed so that it can be the single
source of default values which has three benefits
 - we don't have to fallback to default with every use of the value
 - we can keep the values commented out in config which makes it easier
to changelater
 - there is single place to look for the default setting

@mbacovsky mbacovsky force-pushed the mbacovsky:25307_no_defaults branch from 09392d9 to b083903 Oct 26, 2018

@mbacovsky mbacovsky force-pushed the mbacovsky:25307_no_defaults branch from b083903 to a12a45b Oct 26, 2018

@mbacovsky

This comment has been minimized.

Copy link
Member Author

mbacovsky commented Oct 26, 2018

@tstrachota, @ofedoren: comments addressed in separate commit. Is it better?
Please squash before merging ;)

@tstrachota

This comment has been minimized.

Copy link
Member

tstrachota commented Nov 1, 2018

Looks ok now. Just one question regarding the design @mbacovsky:
Have you considered just adding equivalent of unless context[:use_defaults] here https://github.com/theforeman/hammer-cli/blob/master/lib/hammer_cli/abstract.rb#L250 ? It seems as the most straightforward solution to me. It would probably also allow keeping the defaults commands' functionality on even with disabled defaults.

@mbacovsky

This comment has been minimized.

Copy link
Member Author

mbacovsky commented Nov 2, 2018

@tstrachota updated. I removed the restriction to save the defaults when disabled. I also moved the logic to the option source.

@mbacovsky

This comment has been minimized.

Copy link
Member Author

mbacovsky commented Nov 2, 2018

@tstrachota updated. Last version?

@tstrachota

This comment has been minimized.

Copy link
Member

tstrachota commented Nov 2, 2018

Looks good and works as expected now. Thanks @mbacovsky

@tstrachota tstrachota merged commit f9b27dc into theforeman:master Nov 2, 2018

1 check passed

hammer Build finished. 2016 tests run, 0 skipped, 0 failed.
Details

mbacovsky added a commit that referenced this pull request Nov 6, 2018

Fixes #25307 - Add ability to disable defaults (#293)
Fixes #25307 - Add ability to disable defaults

Processing of defaults can be altered in cli_config.yml with
:use_defaults: true/false or on CLI with --use-defaults
--no-use-defaults respectively. both ways can be combined.
The value is propagated in the context as :use_defaults.
The defaults are enabled by default.

Hammer settings class was changed so that it can be the single
source of default values which has three benefits
 - we don't have to fallback to default with every use of the value
 - we can keep the values commented out in config which makes it easier
to change later
 - there is single place to look for the default setting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.