Skip to content

Fixes #19676 - saving defaults from hammer shell#244

Merged
mbacovsky merged 2 commits intotheforeman:masterfrom
tstrachota:defaults_shell_19676
Aug 4, 2017
Merged

Fixes #19676 - saving defaults from hammer shell#244
mbacovsky merged 2 commits intotheforeman:masterfrom
tstrachota:defaults_shell_19676

Conversation

@tstrachota
Copy link
Member

See the linked redmine issue for reproducer steps.

@mbacovsky
Copy link
Member

@tstrachota, the patch looks good. But I'm not sure it fixes the whole problem. When updating defaults in the hammer shell, the hammer instance defaults are not updated. Just the config file.

hammer> defaults list
----------------|---------------------
PARAMETER       | VALUE               
----------------|---------------------
----------------|---------------------

hammer> defaults add --param-name location-id --provider foreman
Added location-id default-option with value that will be generated from the server.

hammer> defaults list
----------------|---------------------
PARAMETER       | VALUE               
----------------|---------------------
----------------|---------------------

The config looks good now and after reloading the shell all is right.

$ cat ~/.hammer/defaults.yml 
---
:defaults:
  :location-id:
    :provider: foreman

The way how the update_defaults_file works is weird. It loads the config, update it and store regardless what the instance content is (settings can be provided in constructor so it can be completely different from the config file content). While the defaults instance in the shell persists it is not reloaded and miss the new data. I'd suggest to change the update_defaults_file to update @defaults_settings and overwrite the config file with it. What do you think?

Copy link
Member

@mbacovsky mbacovsky left a comment

Choose a reason for hiding this comment

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

I've added my concerns in the main PR conversation

@tstrachota
Copy link
Member Author

@mbacovsky added a commit that fixes behavior in shell. Should be squashed before merge.

@ares
Copy link
Member

ares commented Jul 21, 2017

Fixes the problem for me, code looks good to me

@tstrachota
Copy link
Member Author

@mbacovsky I've addressed your comments, can you please re-review?

@mbacovsky
Copy link
Member

👍 Thanks @tstrachota, 🔨 🐚 works great now!

@mbacovsky mbacovsky merged commit ff54c3a into theforeman:master Aug 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants