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 #27868 - Add option to support host's param type #448

Merged
merged 1 commit into from Oct 25, 2019

Conversation

@ofedoren
Copy link
Member

ofedoren commented Sep 20, 2019

Requires theforeman/hammer-cli#316

Or we could remove :host_parameters_attributes from

:puppet_class_ids, :host_parameters_attributes, :interfaces_attributes, :root_pass]
to build the option from apidocs and thus the core changes will be not required.

Note: if both --parameters and --typed-parameters are specified then the values from --parameters are not overrided by values from --typed-parameters.

Copy link
Contributor

shiramax left a comment

thanks @ofedoren !
why you create a new option for typed parameters and not replacing the parameters option?
Also, It's not clear from the description how we should send the name, type, and value of the parameter.

@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Oct 7, 2019

@shiramax, thanks for review! :)

I'm adding a new option because of parsing the values provided for that option(s).

For example option --parameters has a key=value normalizer, so it accepts a list of key=value only:

hammer host update --parameters "par1=val1,par2=val2"

But option --typed-parameters has a bit more complex normalizer which allows to pass a bit more comlex value defined by a schema:

hammer host update --typed-parameters "name=par1\,value=val1\,parameter_type=string\,hidden_value=false,name=par2\,value=val2\,parameter_type=string\,hidden_value=false"

Please notice that we cannot pass the parameter_type for --parameters option, but we can for --typed-parameters. Also, we cannot just wipe --parameters option or change it to have normalizer like --typed-parameters does since it could break automatization used in scripts.

@shiramax

This comment has been minimized.

Copy link
Contributor

shiramax commented Oct 10, 2019

Thanks for the explanation @ofedoren, I think this input of the typed-parameter, can be a bit confusing: "name=par1,value=val1,parameter_type=string,hidden_value=false,name=par2,value=val2,parameter_type=string,hidden_value=false"
"

I wonder if we could to do a different format for this, for example, maybe we could do this like we do the --interfaces or --volume. it means --typed-parameters should be a parameter we could speficy multiple times. also we will need to generate help for the parameters, and finally you should change the parameters that send to the server.
It might be more work, but I think it would be more clear how to use the method.

Copy link
Member

mbacovsky left a comment

👍 it seems good to merge and solves the issue.

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Oct 25, 2019

I quite like the solution that @shiramax suggested. That would be probably easier to input manually for the users. For the solution provided in this PR I can see benefits for automation where the parameters are added in scripts especially in JSON format.

It seems these two ways are not conflicting and there is chance to add also the multivalue variant @shiramax suggested. I assume the name of the option would be --typed-parameter (singular) so there is no conflict. It may be confusing if it is not consistent though.
@shiramax sorry for merging without your approval but it seems your questions were answered and this needs to be part of 1.24. If you have any concerns, please let me know and we can resolve this in minor release.

@mbacovsky mbacovsky merged commit 3288835 into theforeman:master Oct 25, 2019
2 checks passed
2 checks passed
Redmine issues Valid issues
Details
hammer Build finished. 5337 tests run, 0 skipped, 0 failed.
Details
ofedoren added a commit that referenced this pull request Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.