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

bug 1614567: Allow user to select Network Port for provisioning #18303

Merged
merged 1 commit into from
Feb 23, 2019

Conversation

sseago
Copy link
Contributor

@sseago sseago commented Dec 19, 2018

Move dialog to provider repo manageiq-providers-openstack

Depends on ManageIQ/manageiq-providers-openstack#416

@mansam
Copy link
Contributor

mansam commented Jan 9, 2019

Hi @agrare, do you know who would be best to review this?

@@ -173,6 +182,15 @@
:required_method: :validate_cloud_network
:display: :edit
:data_type: :integer
:network_port:
:values_from:
:method: :allowed_network_ports
Copy link
Member

Choose a reason for hiding this comment

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

I don't see this method in the manageiq code base

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It exists in manageiq-providers-openstack (once the associated PR is pushed). This is an openstack-specific yaml file

:description: Network Port
:auto_select_single: false
:required: true
:required_method: :validate_network_port
Copy link
Member

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It exists in manageiq-providers-openstack (once the associated PR is pushed). This is an openstack-specific yaml file. Note that the same thing is done for validate_cloud_network already (only exists in the openstack provider), which was what I was following when implementing this.

@mansam
Copy link
Contributor

mansam commented Jan 28, 2019

hi @bdunne @lfu, anything else needed from @sseago?

@gmcculloug
Copy link
Member

@sseago @mansam PR ManageIQ/manageiq-providers-openstack#416 needs to be merged first since this PR relies on the methods allowed_network_ports and validate_network_port.

@@ -164,6 +164,15 @@
:required: true
:display: :edit
:data_type: :integer
:cloud_network_choice_type:
Copy link
Member

Choose a reason for hiding this comment

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

Suggest using cloud_network_type. Since these are dialog options aren't all fields chosen "choice" values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cloud_network_type would be misleading. What is being selected here is not a type of network, but the method of selecting the network. I'm open to other words, though, if you think choice is confusing. cloud_network_identification_method perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My suggestion above is a bit too long. I'm going to change it to cloud_network_selection_method when I move it into the openstack repo.

@gmcculloug
Copy link
Member

@sseago @mansam Another thing to consider is moving the dialog to the OpenStack provider repo like what was done for Azure. Ultimately it looks like all this logic could be moved to the provider specific repo.

The options_helper.rb logic could be moved to the Provider repo as well, unless you think it will be used by other cloud providers.

@sseago
Copy link
Contributor Author

sseago commented Feb 11, 2019

@gmcculloug The reason this was put into the core repo is that's where the dialog currently lives. I don't see any reason why it couldn't be moved to the provider repo, if there's precedent for doing this elsewhere (do you happen to know which core and azure PRs moved this over for azure? if not, I can search for them). As for the options_helper.rb logic, yeah, that should live in whatever repo the dialog definition is in.

@gmcculloug
Copy link
Member

Here's the PR that moved the dialog to the provider repo: ManageIQ/manageiq-providers-azure#16.

fixes https://bugzilla.redhat.com/show_bug.cgi?id=1614567

Allow user to select Network Port or Cloud Network when provisioning
an OpenStack cloud instance.
@sseago
Copy link
Contributor Author

sseago commented Feb 12, 2019

@gmcculloug OK, I've updated this PR to move the whole miq_provision_openstack_dialogs_template.yaml file to the openstack provider.

@miq-bot
Copy link
Member

miq-bot commented Feb 12, 2019

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

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.

This PR is not just a deletion of the dialog in favor of having it in the provider repo. 👍

@gmcculloug gmcculloug merged commit cb18388 into ManageIQ:master Feb 23, 2019
@gmcculloug gmcculloug added this to the Sprint 106 Ending Mar 4, 2019 milestone Feb 23, 2019
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.

6 participants