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 #29254 - add display options to host creation on ovirt #507

Merged
merged 1 commit into from Mar 12, 2020
Merged

Fixes #29254 - add display options to host creation on ovirt #507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 7 additions & 5 deletions lib/hammer_cli_foreman/compute_resource/ovirt.rb
View file
Open in desktop
Expand Up @@ -7,11 +7,13 @@ def name

def compute_attributes
[
['cluster', _('ID or name of cluster to use')],
['template', _('Hardware profile to use')],
['cores', _('Integer value, number of cores')],
['sockets', _('Integer value, number of sockets')],
['memory', _('Amount of memory, integer value in bytes')]
['cluster', _('ID or name of cluster to use')],
['template', _('Hardware profile to use')],
['cores', _('Integer value, number of cores')],
['sockets', _('Integer value, number of sockets')],
['memory', _('Amount of memory, integer value in bytes')],
['display_type', _('Possible values: %s') % 'VNC, SPICE'],
['keyboard_layout', _('Possible values: %s. Not usable if display type is SPICE.') % 'ar, de-ch, es, fo, fr-ca, hu, ja, mk, no, pt-br, sv, da, en-gb, et, fr, fr-ch, is, lt, nl, pl, ru, th, de, en-us, fi, fr-be, hr, it, lv, nl-be, pt, sl, tr']
]
end

Expand Down
8 changes: 8 additions & 0 deletions lib/hammer_cli_foreman/hosts/common_update_options.rb
View file
Open in desktop
Expand Up @@ -79,6 +79,14 @@ def request_params
params['host']['host_parameters_attributes'] ||= option_typed_parameters unless option_typed_parameters.nil?
params['host']['compute_attributes'] = option_compute_attributes || {}

compute_attributes = params['host']['compute_attributes']
compute_attributes['display'] = {} unless compute_attributes['display_type'].nil? && compute_attributes['keyboard_layout'].nil?
compute_attributes['display']['type'] = compute_attributes['display_type'] unless compute_attributes['display_type'].nil?
compute_attributes['display']['keyboard_layout'] = compute_attributes['keyboard_layout'] unless compute_attributes['keyboard_layout'].nil?
compute_attributes.delete('display_type')
compute_attributes.delete('keyboard_layout')
Copy link
Member

Choose a reason for hiding this comment

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

It looks like I cannot use keyboard_layout without display_type being set. E.g. if I set keyboard_layout only, it will be ignored and not passed into params under ['host']['compute_attributes']. Is that expected behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, actually only if the display type is VNC there should not be an option to choose the key_board layout according to the web UI. I will fix it.

Copy link
Member

Choose a reason for hiding this comment

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

After update I see that I can pass display_type or keyboard_layout independently 👍. But there is no mention that a user shouldn't pass keyboard_layout if he/she passes display_type = VNC. Honestly I don't know if it is a problem, my concern is based on your previous comment only.

If we want to address it as well, then I'd expand a bit the help: 'keyboard_layout', _('Possible values: %s. Not usable if display type is VNC.') %... or something similar.
OR
We could add display type check in the code and warn the user if he/she passes display_type = VNC and keyboard_layout = anything.

What do you think? Otherwise the PR is ready to merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think expanding the help is a good idea.
I also made a mistake with what I wrote. Actually it is possible to add keybaord_layout only when the display_type is VNC. When the display_type is SPICE (the other option) it should not be added.
I will add a short explanation to the help. :)
Thanks!

params['host']['compute_attributes'] = compute_attributes

if action == :update
params['host']['compute_attributes']['volumes_attributes'] = nested_attributes(option_volume_list) unless option_volume_list.empty?
params['host']['interfaces_attributes'] = interfaces_attributes unless option_interface_list.empty?
Expand Down