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

Validate either a host or cluster is selected when provisioning on VMware #11665

Merged
merged 1 commit into from Oct 5, 2016
Merged

Validate either a host or cluster is selected when provisioning on VMware #11665

merged 1 commit into from Oct 5, 2016

Conversation

agrare
Copy link
Member

@agrare agrare commented Oct 3, 2016 •
edited

VMware allows you to select just a cluster (and not a host) if that cluster supports DRS, this updates the validation method to check that:

  1. A host or a cluster is explicitly selected
  2. The selected cluster has DRS enabled if no host is selected

screenshot from 2016-10-04 19-13-07
screenshot from 2016-10-04 19-12-49

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

@agrare
Copy link
Member Author

agrare commented Oct 3, 2016

cc @gmcculloug @bdunne

@agrare agrare force-pushed the vmware_infra_provisioning_cluster_validation branch 2 times, most recently from ae4509b to cfa67a9 Compare October 4, 2016 14:33
@agrare agrare changed the title [WIP] Validate either a host or cluster is selected when provisioning on VMware Validate either a host or cluster is selected when provisioning on VMware Oct 4, 2016
@agrare agrare removed the wip label Oct 4, 2016
bdunne
bdunne requested changes Oct 4, 2016
View reviewed changes
Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

Overall, this looks like it's on the right track. Thanks @agrare

...models/manageiq/providers/vmware/infra_manager/provision_workflow/dialog_field_validation.rb

ems_cluster = EmsCluster.find_by(:id => get_value(values[:placement_cluster_name]))

if get_value(values[:placement_cluster_name]).blank? || ems_cluster.nil?
Copy link
Member

@bdunne bdunne Oct 4, 2016 •
edited

Choose a reason for hiding this comment

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

Wouldn't it be enough to say if ems_cluster.nil? rather than calling get_value a second time?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it would, done

...models/manageiq/providers/vmware/infra_manager/provision_workflow/dialog_field_validation.rb
if get_value(values[:placement_cluster_name]).blank? || ems_cluster.nil?
_("Either Host Name or Cluster Name is required")
elsif !ems_cluster.drs_enabled
_("Host Name required for Non-DRS enabled cluster")
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this message should be:
_("%{field_required} for Non-DRS enabled cluster") % {:field_required => result}

Copy link
Member Author

@agrare agrare Oct 4, 2016 •
edited

Choose a reason for hiding this comment

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

Done, is there something similar I can do for "Either Host Name or Cluster Name is required"?

Copy link
Member

Choose a reason for hiding this comment

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

The current validation goes field by field and runs each validation method or required method for required fields. Getting the messaging right would be tough.

@agrare agrare force-pushed the vmware_infra_provisioning_cluster_validation branch from cfa67a9 to 84c8bb2 Compare October 4, 2016 20:52
bdunne
bdunne reviewed Oct 4, 2016
View reviewed changes
...models/manageiq/providers/vmware/infra_manager/provision_workflow/dialog_field_validation.rb
if ems_cluster.nil?
_("Either Host Name or Cluster Name is required")
elsif !ems_cluster.drs_enabled
_("%{field_required} required for Non-DRS enabled cluster") % {:field_required => result}
Copy link
Member

Choose a reason for hiding this comment

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

field_required should render out to something like Environment/Host is required, so you should also drop the "required" in your string otherwise you'll get Environment/Host is required required for Non-DRS enabled cluster

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@agrare agrare force-pushed the vmware_infra_provisioning_cluster_validation branch from 84c8bb2 to 449deb8 Compare October 4, 2016 23:14
@miq-bot
Copy link
Member

miq-bot commented Oct 4, 2016

Checked commit agrare@449deb8 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 🍰

@bdunne bdunne merged commit 5ef33b1 into ManageIQ:master Oct 5, 2016
@bdunne bdunne added this to the Sprint 48 Ending Oct 24, 2016 milestone Oct 5, 2016
@agrare agrare deleted the vmware_infra_provisioning_cluster_validation branch October 5, 2016 13:55
@bdunne
Copy link
Member

bdunne commented Oct 5, 2016

@agrare Please apply the appropriate euwe label

@agrare agrare added the euwe/yes label Oct 5, 2016
chessbyte pushed a commit that referenced this pull request Oct 5, 2016
@bdunne @chessbyte
Merge pull request #11665 from agrare/vmware_infra_provisioning_clust…
ac19cd3 
…er_validation

Validate either a host or cluster is selected when provisioning on VMware
(cherry picked from commit 5ef33b1)
@chessbyte
Copy link
Member

Euwe Backport details:

$ git log
commit ac19cd3bc3a59bc314854cb28b6125f814aed434
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Wed Oct 5 09:54:44 2016 -0400

    Merge pull request #11665 from agrare/vmware_infra_provisioning_cluster_validation

    Validate either a host or cluster is selected when provisioning on VMware
    (cherry picked from commit 5ef33b185bfc6f961089595ef5ab4b142efcc45c)

@agrare
Copy link
Member Author

agrare commented Oct 7, 2016

Darga BZ https://bugzilla.redhat.com/show_bug.cgi?id=1378116

Backport after #11437

chessbyte pushed a commit that referenced this pull request Oct 11, 2016
@bdunne @chessbyte
Merge pull request #11665 from agrare/vmware_infra_provisioning_clust…
8263d1b 
…er_validation

Validate either a host or cluster is selected when provisioning on VMware
(cherry picked from commit 5ef33b1)

https://bugzilla.redhat.com/show_bug.cgi?id=1378116
@chessbyte
Copy link
Member

Darga Backport details:

$ git log
commit 8263d1b798be931ce5bc53ee6015dd2b26538a05
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Wed Oct 5 09:54:44 2016 -0400

    Merge pull request #11665 from agrare/vmware_infra_provisioning_cluster_validation

    Validate either a host or cluster is selected when provisioning on VMware
    (cherry picked from commit 5ef33b185bfc6f961089595ef5ab4b142efcc45c)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdunne bdunne bdunne approved these changes

Assignees

@gmcculloug gmcculloug

Labels
darga/backported enhancement euwe/backported lifecycle/provisioning providers/infrastructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@agrare @miq-bot @bdunne @chessbyte @Fryguy @gmcculloug