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 allowed_subnets for scvmm provisioning #16177

Merged
merged 1 commit into from
Oct 30, 2017

Conversation

agrare
Copy link
Member

@agrare agrare commented Oct 11, 2017

Allow subnet selection for SCVMM provisioning

Depends on:

Subnets dropdown:
31506443-72ab6936-af45-11e7-8ef9-30b97012f9b7

Logical networks and VM Networks:
screenshot from 2017-10-16 15-07-46

@agrare agrare changed the title Add allowed_subnets for scvmm provisioning [WIP] Add allowed_subnets for scvmm provisioning Oct 11, 2017
@agrare agrare added the wip label Oct 11, 2017
@bronaghs
Copy link

Hi @agrare
I am wondering if this is generic enough to remain here or should this be moved to the SCVMM subclass here ?

@agrare
Copy link
Member Author

agrare commented Oct 12, 2017

@bronaghs I went back and forth on that myself and I could still go either way. Currently it will only be shown for SCVMM but it is generic enough to work for any provider if/when they add subnet support which is why I had it here.

@bronaghs
Copy link

Thats fine by me. We can always move it in the future if we have to.

@bronaghs
Copy link

This is no longer a wip, right?

@bronaghs
Copy link

@miq-bot assign @blomquisg

@agrare
Copy link
Member Author

agrare commented Oct 13, 2017

@bronaghs I still need to filter the subnets and vlans if the other is selected and update the name for vm_networks to include the parent lan

@agrare agrare force-pushed the scvmm_provision_subnets branch 3 times, most recently from fe1bbdf to 701d36b Compare October 16, 2017 20:31
@agrare agrare changed the title [WIP] Add allowed_subnets for scvmm provisioning Add allowed_subnets for scvmm provisioning Oct 17, 2017
@agrare agrare removed the wip label Oct 17, 2017
@agrare
Copy link
Member Author

agrare commented Oct 17, 2017

@gmcculloug can you or @lfu help review the changes?
cc @bronaghs @djberg96 this should be ready now

The biggest nuisance is that if there is only one subnet it is auto-selected, not sure if I need to change that here or in the UI pr.

@gmcculloug
Copy link
Member

@agrare Add :auto_select_single: false to the dialog field to avoid the auto-select issue.

Example: https://github.com/ManageIQ/manageiq/blob/master/product/dialogs/miq_dialogs/miq_provision_vmware_dialogs_template.yaml#L158

@agrare
Copy link
Member Author

agrare commented Oct 17, 2017

❤️ thanks @gmcculloug

@agrare
Copy link
Member Author

agrare commented Oct 18, 2017

I moved the bulk of the vlan and subnet handling into the scvmm provision_workflow because changing the lan key to uid_ems could break other providers that rely on it being the name.

This is moved here ManageIQ/manageiq-providers-scvmm#27

@@ -249,6 +249,10 @@ def load_hosts_vlans(hosts, vlans)
end
end

def allowed_subnets(_options = {})
Copy link
Member

Choose a reason for hiding this comment

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

This method will only be called by a dialog that defines the method name in :values_from on a dialog field and the subclassed provider workflow would include the method.

If there is common code that all, or most, workflows share it would be alright to include it here, but this not needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 removed

Copy link
Member

@gmcculloug gmcculloug left a comment

Choose a reason for hiding this comment

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

We chopped plenty out of this PR, don't want to push my luck. 👍

@lfu
Copy link
Member

lfu commented Oct 18, 2017

What happen to :auto_select_single: false? Do we still want to avoid the auto-select issue?

@agrare
Copy link
Member Author

agrare commented Oct 18, 2017

Ah good catch @lfu I mistakenly dropped that commit

@blomquisg
Copy link
Member

@agrare we're still waiting on ManageIQ/manageiq-providers-scvmm#27 ?

@agrare
Copy link
Member Author

agrare commented Oct 23, 2017

@blomquisg correct, it was blocked by ManageIQ/manageiq-providers-scvmm#29 which was just merged.
Need to re-test ManageIQ/manageiq-providers-scvmm#27 now.

@agrare agrare force-pushed the scvmm_provision_subnets branch 2 times, most recently from 05cd419 to 240e63e Compare October 25, 2017 19:26
@miq-bot
Copy link
Member

miq-bot commented Oct 26, 2017

Checked commit agrare@f5f85aa with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. 🍰

@agrare
Copy link
Member Author

agrare commented Oct 30, 2017

@gmcculloug @blomquisg all dependent PRs have been merged can you take a look?

@blomquisg blomquisg merged commit 7f84757 into ManageIQ:master Oct 30, 2017
@blomquisg blomquisg added this to the Sprint 72 Ending Oct 30, 2017 milestone Oct 30, 2017
@agrare agrare deleted the scvmm_provision_subnets branch October 30, 2017 15:41
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.

7 participants