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.

Sign up for GitHub

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

Jump to bottom

Fixes #18878 - Adding hidden_value option to parameters #290

Merged
merged 1 commit into from Mar 21, 2017
Merged

Fixes #18878 - Adding hidden_value option to parameters #290

merged 1 commit into from Mar 21, 2017

Conversation

orrabin
Copy link
Member

@orrabin orrabin commented Mar 13, 2017

No description provided.

@orrabin
Copy link
Member Author

orrabin commented Mar 13, 2017 •
edited

This works for global parameters but it doesn't hide host parameters yet.
Update: works now, was missing it in create_parameter.
Note: the update still requires a value so a parameter can't currently update the hide/unhide only, should this be changed?

@mbacovsky mbacovsky self-assigned this Mar 13, 2017
@orrabin orrabin force-pushed the 18878 branch 2 times, most recently from b9c8fa5 to 6564816 Compare March 13, 2017 15:09
mbacovsky
mbacovsky suggested changes Mar 13, 2017
View reviewed changes
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.

Besides the in-line comments the code looks good.

lib/hammer_cli_foreman/common_parameter.rb Outdated
@@ -26,6 +26,7 @@ class SetCommand < HammerCLIForeman::Command

option "--name", "NAME", _("parameter name"), :required => true
option "--value", "VALUE", _("parameter value"), :required => true
option "--hidden_value", "HIDDEN VALUE", _("should the value be hidden")
Copy link
Member

Choose a reason for hiding this comment

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

Options should have dashes only: --hidden-value. Multi-word param names should be connected with underscore: HIDDEN_VALUE.

mbacovsky
mbacovsky reviewed Mar 13, 2017
View reviewed changes
lib/hammer_cli_foreman/parameter.rb Outdated
@@ -48,6 +48,7 @@ def self.create_option_builder
class SetCommand < AbstractParameterCommand
option "--name", "NAME", _("parameter name"), :required => true
option "--value", "VALUE", _("parameter value"), :required => true
option "--hidden_value", "HIDDEN VALUE", _("should the value be hidden")
Copy link
Member

Choose a reason for hiding this comment

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

dtto (dash and underscore missing)

@orrabin
Copy link
Member Author

orrabin commented Mar 13, 2017

@mbacovsky thanks, fixed

mbacovsky
mbacovsky suggested changes Mar 14, 2017
View reviewed changes
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.

While testing I found out one usability issue - the new option should be of type boolean so that the the allowed values are shown in the help and the value is validated.

lib/hammer_cli_foreman/common_parameter.rb Outdated
@@ -26,6 +26,7 @@ class SetCommand < HammerCLIForeman::Command

option "--name", "NAME", _("parameter name"), :required => true
option "--value", "VALUE", _("parameter value"), :required => true
option "--hidden-value", "HIDDEN_VALUE", _("should the value be hidden")
Copy link
Member

Choose a reason for hiding this comment

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

While testing I found out one usability issue - the new option should be of type boolean so that the the allowed values are shown in the help and the value is validated.

      option "--hidden-value", "HIDDEN_VALUE", _("should the value be hidden"),
        :format => HammerCLI::Options::Normalizers::Bool.new

lib/hammer_cli_foreman/parameter.rb Outdated
@@ -48,6 +48,7 @@ def self.create_option_builder
class SetCommand < AbstractParameterCommand
option "--name", "NAME", _("parameter name"), :required => true
option "--value", "VALUE", _("parameter value"), :required => true
option "--hidden-value", "HIDDEN_VALUE", _("should the value be hidden")
Copy link
Member

Choose a reason for hiding this comment

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

dtto - boolean

@mbacovsky
Copy link
Member

@orrabin, Thanks for updating. I requested one more change that I found out during my testing.

@orrabin
Copy link
Member Author

orrabin commented Mar 20, 2017 •
edited

@mbacovsky I added tests but they are not working yet. Adding hidden-value to the api mock resulted an empty string instead of an empty hash sent to print_message which results in the error: TypeError: no implicit conversion of Symbol into Integer.

Edit: @mbacovsky fixed the test as you suggested on IRC, everything is passing locally.

@mbacovsky
Copy link
Member

👍 Thanks @orrabin, it looks and works great now! Merging... 💐

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

@mbacovsky mbacovsky mbacovsky approved these changes

Assignees

@mbacovsky mbacovsky

Labels
Needs re-review Needs testing
Projects
None yet
Milestone
No milestone
3 participants
@orrabin @mbacovsky @theforeman-bot