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

Add sysprep support for ovirt #275

Merged
merged 1 commit into from
Sep 17, 2018
Merged

Conversation

borod108
Copy link
Contributor

@borod108 borod108 commented Aug 5, 2018

Add sysprep specification support for vm provisioning from template
for the oVirt provider.

Depends on: ManageIQ/manageiq#17636
Required for: ManageIQ/manageiq-ui-classic#4211
Implements: https://bugzilla.redhat.com/show_bug.cgi?id=1553833

Copy link
Contributor

@pkliczewski pkliczewski left a comment

Choose a reason for hiding this comment

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

@borod108 please add a spec so we can make sure that sysprep flow works correctly

@@ -49,6 +51,10 @@ def configure_container

private

def sysprep_specification_selected?
options.dig(:sysprep_enabled,1) == "Sysprep Specification"
Copy link
Contributor

Choose a reason for hiding this comment

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

sysprep_option = get_option(:sysprep_enabled)

Does sysprep_option contain fields or "Sysprep Specification"? Can we replace dig with it?

@@ -21,8 +21,11 @@ def configure_cloud_init
end

def configure_sysprep
content = get_option(:sysprep_upload_text)

if options[:sysprep_enabled][1] == "Sysprep Specification"
Copy link
Contributor

@pkliczewski pkliczewski Aug 6, 2018

Choose a reason for hiding this comment

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

It looks like we could use sysprep_specification_selected? here.

BTW This line cause travis to fail with:

undefined method `[]' for nil:NilClass

@borod108 borod108 force-pushed the rfe/sysprep branch 2 times, most recently from 6313933 to 97f17f2 Compare August 9, 2018 07:13

def prepare_customization_template_substitution_options
substitution_options = super
substitution_options[:timezone] = extract_timezone(substitution_options[:timezone])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agrare this is not nice :( perhaps you know a better way to do it? (anyway I can do it in the automation/ui side somehow defining I want only the value without the key? currently it returns ["300", "(GMT+13:00) Nuku'alofa"])

Copy link
Member

Choose a reason for hiding this comment

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

Hm I think the automate workflow should use 300 as the key and pass down the value /cc @gmcculloug ??

Copy link
Member

Choose a reason for hiding this comment

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

Fields displayed as drop-downs in the UI are stored as an array with [key, display_name], which is what you are seeing here.

Values like this in the options hash are normally processed in the backend using the get_option method which handles parsing the field and only returning the first key if it is an array. For VMware, which is the only other provider to use a timezone drop-down today, this is handled in the backend model and the parsed value is passed to the provider.

If the only thing you need to pass for substitution is the key then I think you would do:
substitution_options[:timezone] = get_option(substitution_options[:timezone])

If that works for you a small refactoring of this would be:

def prepare_customization_template_substitution_options
  super.tap do |substitution_options|
    substitution_options[:sysprep_timezone] = get_option(substitution_options[:sysprep_timezone])
  end
end

If you think you need to pass both key and display_name you could setup a second key here as well and there is a get_option_last helper method. For example:

substitution_options[:sysprep_timezone_name] = get_option_last(substitution_options[:sysprep_timezone])

Copy link
Member

Choose a reason for hiding this comment

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

@borod108 Did my suggestion not work out? I'm curious about the details.

@borod108 borod108 force-pushed the rfe/sysprep branch 2 times, most recently from d0a77c2 to 4c1cb91 Compare September 16, 2018 12:13
@borod108 borod108 closed this Sep 16, 2018
@borod108 borod108 reopened this Sep 16, 2018
Add sysprep specification support for vm provisioning from template
for the oVirt provider.

Depends on: ManageIQ/manageiq#17636
Required for: ManageIQ/manageiq-ui-classic#4211
Implements: https://bugzilla.redhat.com/show_bug.cgi?id=1553833
@miq-bot
Copy link
Member

miq-bot commented Sep 16, 2018

Checked commit borod108@0a82432 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍪

@agrare agrare merged commit 01a674b into ManageIQ:master Sep 17, 2018
@agrare agrare added this to the Sprint 95 Ending Sep 24, 2018 milestone Sep 17, 2018
@borod108 borod108 deleted the rfe/sysprep branch December 4, 2018 14:57
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.

5 participants