Add a virtual column for `supports_block_storage?` and `supports_cloud_object_store_container_create?` #15600

AparnaKarve · 2017-07-19T00:18:08Z

The following API (that used to work before), returns an empty list currently since APIs do not support filtering on non-column methods anymore.

/api/providers?expand=resources&attributes=id,name&filter[]=supports_block_storage?=true

The solution is to add a new virtual column: v_supports_block_storage for supports_block_storage?.
And while at it, the ? was dropped from the original name to make it more friendly in the API-based URLs

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

AparnaKarve · 2017-07-19T00:19:46Z

@imtayadeway Please review. Thanks.

EDIT: Note that alias_method :v_supports_block_storage, :supports_block_storage? does not work hence I had to write a method for v_supports_block_storage

Once the review is done, I also need to make identical changes for supports_cloud_object_store_container_create?

imtayadeway

Looks great! Just one request!

Was this the only column that needed to be added?

imtayadeway · 2017-07-19T14:47:14Z

app/models/ext_management_system.rb

@@ -143,6 +143,7 @@ def hostname_uniqueness_valid?
  virtual_column :total_vms_never,         :type => :integer
  virtual_column :total_vms_suspended,     :type => :integer
  virtual_total  :total_subnets,           :cloud_subnets
+  virtual_column :v_supports_block_storage, :type => :boolean


IIRC, I think we were trying to prevent adding further v_ methods, so this could be :supports_block_storage

Ok, not a problem. I can make it :supports_block_storage.

AparnaKarve · 2017-07-19T14:55:36Z

Was this the only column that needed to be added?

@imtayadeway You are correct, there is one more column.
I wanted to get :supports_block_storage reviewed first and then apply identical changes for :supports_cloud_object_store_container_create.

imtayadeway · 2017-07-19T18:18:52Z

app/models/ext_management_system.rb

@@ -542,6 +544,14 @@ def total_vms_never;     vm_count_by_state("never");     end

  def total_vms_suspended; vm_count_by_state("suspended"); end

+  def supports_block_storage


@durandom any idea why alias would not work here? With alias supports_block_storage supports_block_storage?:

ems.supports_block_storage? # => true ems.supports_block_storage # => false

maybe because you've added the alias before the block storage mixing got included?
the supports_feature_mixin defaults all features to unsupported. once it's supported it defines another method.

I tried to remove this in #11322 - but unfortunately that PR got stale

imtayadeway

Thanks @AparnaKarve ! 🍰

durandom

is that the case for all other features in the supports feature mixin?
I mean, should they be queryable via the REST api?

If so, I would add those virtual columns in the supports_feature_mixin

durandom · 2017-07-19T18:42:57Z

app/models/ext_management_system.rb

@@ -542,6 +544,14 @@ def total_vms_never;     vm_count_by_state("never");     end

  def total_vms_suspended; vm_count_by_state("suspended"); end

+  def supports_block_storage


maybe because you've added the alias before the block storage mixing got included?
the supports_feature_mixin defaults all features to unsupported. once it's supported it defines another method.

I tried to remove this in #11322 - but unfortunately that PR got stale

durandom · 2017-07-19T18:43:57Z

spec/factories/ext_management_system.rb

@@ -234,6 +234,18 @@
    provider_region "us-east-1"
  end

+  factory :ems_amazon_storage_manager_ebs,


I would try not to add more provider specific factories. See below for another solution to test this

durandom · 2017-07-19T18:46:49Z

spec/models/ext_management_system_spec.rb

+
+  context "virtual column :supports_block_storage" do
+    it "returns true for Amazon EBS" do
+      ems_amz_ebs = FactoryGirl.create(:ems_amazon_storage_manager_ebs)


How about not using an amazon ems, but just a mock?

context "virtual column :supports_block_storage" do it "returns true if block storage is supported" do ems = FactoryGirl.build(:ems) allow(ems).to receive(:supports_block_storage?).and.return(true) expect(ems_amz_ebs.supports_block_storage).to eq(true) end end

Alright, if you recommend that.
I'm OK either ways - mock or not :-)

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

miq-bot · 2017-07-19T20:07:53Z

Checked commits AparnaKarve/manageiq@91b2e0f~...35a5f2c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

AparnaKarve · 2017-07-19T20:26:08Z

I would add those virtual columns in the supports_feature_mixin

@durandom That will not work. Getting this --

~/manageiq/app/models/mixins/supports_feature_mixin.rb:126:in `<module:SupportsFeatureMixin>': undefined method `virtual_column' for SupportsFeatureMixin:Module (NoMethodError)

If I'm not mistaken, you can use virtual_column only in the actual AR class.
(but I could be wrong)
Any suggestions on how to resolve the above error?

durandom · 2017-07-20T07:50:58Z

I've created #15614 as a more generalized solution

bronaghs · 2017-07-24T17:16:18Z

@miq-bot assign @blomquisg

blomquisg · 2017-07-27T16:38:41Z

I'll merge this for the bug fixes. We can continue the conversation in #15614 as a possible refactoring effort.

simaishi · 2017-07-28T14:20:53Z

@AparnaKarve ManageIQ/manageiq-ui-classic#1711 depends on this PR and that's marked as fine/yes. I assume this PR should be fine/yes as well?

AparnaKarve · 2017-07-28T14:26:55Z

@simaishi Yes. This needs to be fine/yes as well.

Add a virtual column for `supports_block_storage?` and `supports_cloud_object_store_container_create?` (cherry picked from commit d7611ab) https://bugzilla.redhat.com/show_bug.cgi?id=1478571 https://bugzilla.redhat.com/show_bug.cgi?id=1479802

simaishi · 2017-08-09T12:29:48Z

Fine backport details:

$ git log -1
commit ccbbe811bcce150e428ecb4a6d2999b40b171ba1
Author: Greg Blomquist <[email protected]>
Date:   Thu Jul 27 12:38:45 2017 -0400

    Merge pull request #15600 from AparnaKarve/bz1471162
    
    Add a virtual column for `supports_block_storage?` and `supports_cloud_object_store_container_create?`
    (cherry picked from commit d7611ab2ed3163ba7a89119d7ea17023aae784ec)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1478571
    https://bugzilla.redhat.com/show_bug.cgi?id=1479802

Add a virtual column for `supports_block_storage?` and `supports_cloud_object_store_container_create?` (cherry picked from commit d7611ab) https://bugzilla.redhat.com/show_bug.cgi?id=1478571 https://bugzilla.redhat.com/show_bug.cgi?id=1479802

AparnaKarve mentioned this pull request Jul 19, 2017

Modify the API params to support filtering on non-column methods ManageIQ/manageiq-ui-classic#1711

Merged

AparnaKarve force-pushed the bz1471162 branch 2 times, most recently from 957a751 to a0a2a4c Compare July 19, 2017 01:37

chessbyte assigned bronaghs Jul 19, 2017

imtayadeway suggested changes Jul 19, 2017

View reviewed changes

AparnaKarve force-pushed the bz1471162 branch 2 times, most recently from 5a133dc to 42dce7e Compare July 19, 2017 18:11

AparnaKarve changed the title ~~Add a virtual column for supports_block_storage?~~ Add a virtual column for supports_block_storage? and supports_cloud_object_store_container_create? Jul 19, 2017

imtayadeway reviewed Jul 19, 2017

View reviewed changes

imtayadeway approved these changes Jul 19, 2017

View reviewed changes

durandom suggested changes Jul 19, 2017

View reviewed changes

AparnaKarve added 7 commits July 19, 2017 12:59

Add a spec for virtual_column v_supports_block_storage

91b2e0f

Added a virtual column for v_supports_block_storage

b2d0c01

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

Address rubocop - Avoid single-line method definitions.

f295650

PR feedback - dropping v_ prefix for the newly added virtual col

39799d3

spec for virtual col supports_cloud_object_store_container_create

2806c50

Add a virtual col for supports_cloud_object_store_container_create

5636ee3

PR feedback - Use mocks in lieu of actual storage factories

35a5f2c

AparnaKarve force-pushed the bz1471162 branch from 42dce7e to 35a5f2c Compare July 19, 2017 20:01

durandom mentioned this pull request Jul 20, 2017

add all features as a virtual column #15614

Closed

chessbyte added enhancement providers labels Jul 24, 2017

miq-bot assigned blomquisg and unassigned bronaghs Jul 24, 2017

blomquisg merged commit d7611ab into ManageIQ:master Jul 27, 2017

blomquisg added this to the Sprint 66 Ending Aug 7, 2017 milestone Jul 27, 2017

AparnaKarve deleted the bz1471162 branch July 28, 2017 14:27

simaishi added the fine/yes label Jul 28, 2017

simaishi added fine/backported and removed fine/yes labels Aug 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a virtual column for `supports_block_storage?` and `supports_cloud_object_store_container_create?` #15600

Add a virtual column for `supports_block_storage?` and `supports_cloud_object_store_container_create?` #15600

AparnaKarve commented Jul 19, 2017

AparnaKarve commented Jul 19, 2017 •

edited

Loading

imtayadeway left a comment

imtayadeway Jul 19, 2017

AparnaKarve Jul 19, 2017

AparnaKarve commented Jul 19, 2017

imtayadeway Jul 19, 2017

durandom Jul 19, 2017

imtayadeway left a comment

durandom left a comment

durandom Jul 19, 2017

durandom Jul 19, 2017

durandom Jul 19, 2017

AparnaKarve Jul 19, 2017

miq-bot commented Jul 19, 2017

AparnaKarve commented Jul 19, 2017

durandom commented Jul 20, 2017

bronaghs commented Jul 24, 2017

blomquisg commented Jul 27, 2017

simaishi commented Jul 28, 2017

AparnaKarve commented Jul 28, 2017

simaishi commented Aug 9, 2017

		@@ -542,6 +544,14 @@ def total_vms_never; vm_count_by_state("never"); end

		def total_vms_suspended; vm_count_by_state("suspended"); end

		def supports_block_storage

Add a virtual column for supports_block_storage? and supports_cloud_object_store_container_create? #15600

Add a virtual column for supports_block_storage? and supports_cloud_object_store_container_create? #15600

Conversation

AparnaKarve commented Jul 19, 2017

AparnaKarve commented Jul 19, 2017 • edited Loading

imtayadeway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imtayadeway left a comment

Choose a reason for hiding this comment

durandom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jul 19, 2017

AparnaKarve commented Jul 19, 2017

durandom commented Jul 20, 2017

bronaghs commented Jul 24, 2017

blomquisg commented Jul 27, 2017

simaishi commented Jul 28, 2017

AparnaKarve commented Jul 28, 2017

simaishi commented Aug 9, 2017

Add a virtual column for `supports_block_storage?` and `supports_cloud_object_store_container_create?` #15600

Add a virtual column for `supports_block_storage?` and `supports_cloud_object_store_container_create?` #15600

AparnaKarve commented Jul 19, 2017 •

edited

Loading