Repository navigation
Refactor provider options - #503
Cristi1324 wants to merge 3 commits into
Conversation
b03c3c2 to
d799e6f
Compare
70b1df5 to
d757307
Compare
|
|
||
| nics_info = osmorphing_info.get('nics_info') | ||
| set_dhcp = osmorphing_info.get('nics_set_dhcp', True) | ||
| set_dhcp = osmorphing_parameters.get('set_dhcp', True) |
There was a problem hiding this comment.
This will be properly handled in: https://github.com/cloudbase/coriolis/pull/534/files.
This PR will need to be rebased eventually, and is therefore blocked in the meantime.
6de4af7 to
328497f
Compare
| "title": "Set DHCP", | ||
| "description": ( | ||
| "Sets whether or not to configure the VM to use DHCP " | ||
| "during the OSMorphing stage." |
There was a problem hiding this comment.
Nit:
| "during the OSMorphing stage." | |
| "during the OSMorphing stage. " | |
| "When disabled, guest static IPs will be preserved after the migration." |
| "Keep the guest user accounts during OS morphing. " | ||
| "When false, OS morphing creates the cloud-init user." |
There was a problem hiding this comment.
| "Keep the guest user accounts during OS morphing. " | |
| "When false, OS morphing creates the cloud-init user." | |
| "Prevents the destination cloud provisioning from resetting the pre-existing credentials. " | |
| "When disabled, the destination platform may also create a provisioned cloud guest user." |
| opts = [ | ||
| cfg.ListOpt( | ||
| "cloudbase_init_plugins", | ||
| default=None, |
There was a problem hiding this comment.
Why don't you use the config_defaults from CORE_DESTINATION_OPTIONS as well?
There was a problem hiding this comment.
If we set a default for cloudbase_init_plugins, it will be applied over the provider default because the provider default is not handled in apply_core_destination_overrides. Maybe we should pass enabled_plugins in osmorphing_parameters as cloudbase_init_plugins, but that means that this will no longer be a drop in replacement.
I can still set defaults from CORE_DESTINATION_OPTIONS for the other options.
| LOG = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def merge_core_destination_options(provider_options, option_names=None): |
There was a problem hiding this comment.
Please set type hints for all introduced method args and return types.
| """ | ||
| if plugins is None: | ||
| return None | ||
| if isinstance(plugins, str): |
There was a problem hiding this comment.
At what point does this plugins be a string? IMO it shouldn't reach this point and then make a list out of the string, especially since the schema expects it to be a list.
If that happens, it means a schema validation is either not being called or unfunctional.
All of this logic is supposed to be done by the schema validator.
| "type": "boolean", | ||
| "default": false | ||
| "default": false, | ||
| "description": "Keep the guest user accounts during OS morphing. When false, OS morphing creates the cloud-init user." |
There was a problem hiding this comment.
Please set the new descriptions here as well.
| # Used when dest-env and the destination provider both omit the key. | ||
| # cloudbase_init_plugins = cloudbaseinit.plugins.common.mtu.MTUPlugin,cloudbaseinit.plugins.windows.ntpclient.NTPClientPlugin | ||
| # Disk copy mechanism for destination worker VMs: HTTPS (TCP/5566) or SSH (TCP/22). | ||
| # data_transfer_mechanism = HTTPS |
There was a problem hiding this comment.
since there's not only osmorphing options in here, maybe we should move these somewhere else, and not in osmorphing_conf?
There was a problem hiding this comment.
I'd also like to see these in a separate section, not under DEFUALT. Something like common_target_option.
d8e98bb to
6cc5e62
Compare
6cc5e62 to
b89fa1b
Compare
3 participants
This PR moves common provider options into core so each provider does not have to redefine the same option schema.
Initial common keys are
nics_set_dhcp,data_transfer_mechanism, andcloudbase_init_plugins. Core merges those schemas into every import provider.Also added
retain_user_credentialsas a separate commit.