Refactor service_type to use constants #17681

bzwei · 2018-07-09T18:46:35Z

Introduce new service type internal. This type of service template is intended to be used internally, not directly exposed through catalog. TransformationPlan is the first template to have this type.

Internal template is expected to be a single item, not bundled.

New method ServiceTemplate.public_service_templates excludes all internal templates.

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

cc @h-kataria

[Edit]
It has been decided that we will make a schema change to introduce a new column to flag whether a template is internal.
This PR will remain open for refactoring purpose.

bzwei · 2018-07-10T15:47:50Z

@miq-bot assign @gmcculloug

gmcculloug · 2018-07-10T17:39:45Z

app/models/service_template.rb

@@ -206,19 +206,22 @@ def create_service(service_task, parent_svc = nil)
    svc
  end

+  def self.public_service_templates
+    where.not(:service_type => 'internal')


Suggest we create constants for internal, 'atomic' and 'composite' .

gmcculloug · 2018-07-10T17:48:15Z

app/models/service_template.rb

@@ -206,19 +206,22 @@ def create_service(service_task, parent_svc = nil)
    svc
  end

+  def self.public_service_templates


This should be a scope and moved up with the other scopes here: https://github.com/ManageIQ/manageiq/blob/master/app/models/service_template.rb#L73-L76

Should this logic be incorporated into the existing displayed scope?

This is to exclude Migration Plan Service Templates from Catalogs Explorer. Backend changes to support this are in ManageIQ/manageiq#17681 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1594408

gmcculloug · 2018-07-11T12:59:17Z

app/models/service_template.rb

@@ -207,36 +213,35 @@ def create_service(service_task, parent_svc = nil)
  end

  def set_service_type


@bzwei The logic of this method has been refactored and it would be good to add some tests around it.

Some additional thoughts:

This should be a private method

Change logic to return if immediately if the current service_type is internal and leave the previous logic in place. (Assuming that's why these changes were made.)

Potentially rename method to avoid using "set" prefix which robocop usually warns about.

This is to exclude Migration Plan Service Templates from Catalogs Explorer. Backend changes to support this are in ManageIQ/manageiq#17681 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1594408

gmcculloug · 2018-07-16T12:46:06Z

app/models/service_template_transformation_plan.rb

@@ -49,7 +51,7 @@ def self.create_catalog_item(options, _auth_user = nil)
    enhanced_config_info = validate_config_info(options)
    default_options =  {
      :display      => false,
-      :service_type => 'atomic',
+      :service_type => SERVICE_TYPE_INTERNAL,


Do we need to pass :service_type here if default_value_for is specified?

bdunne

It feels like there should be a ServiceTemplateInternal subclass, but ¯\_(ツ)_/¯

bdunne · 2018-07-16T14:01:08Z

app/models/service_template.rb

@@ -74,6 +79,7 @@ class ServiceTemplate < ApplicationRecord
  scope :without_service_template_catalog_id,       ->         { where(:service_template_catalog_id => nil) }
  scope :with_existent_service_template_catalog_id, ->         { where.not(:service_template_catalog_id => nil) }
  scope :displayed,                                 ->         { where(:display => true) }
+  scope :public_service_templates,                  ->         { where.not(:service_type => 'internal') }


Would just public make more sense?

public is reserved keyword. I would rather not to cause confusion even if it is allowed here.

bdunne · 2018-07-16T14:11:20Z

app/models/service_template.rb

+      svc_type = self.class.SERVICE_TYPE_UNKNOWN
+    else
+      svc_type = self.class::SERVICE_TYPE_ATOMIC
+      service_resources.each do |sr|


Wouldn't this be more efficient as:

service_resources.where(:resource_type => ["Service", "ServiceTemplate"]).any? ? self.class::SERVICE_TYPE_COMPOSITE : self.class::SERVICE_TYPE_ATOMIC

I like your proposed way. However it does not pass the spec test. I realize this method is called after add_resource which is not yet persisted in DB. This method is renamed from set_service_type. Let's keep the old implementation which make no assumption that service_resources have to be loaded from DB.

bdunne · 2018-07-16T14:11:38Z

app/models/service_template.rb

+      end
+    end
+
+    self.service_type = svc_type


Should this save the record?

No. It is a private method. The caller is responsible for saving the record.

gmcculloug · 2018-07-17T16:40:46Z

@bdunne I thought about subclassing as well. I think we would need to establish a new base class model (for example service_template_base) and then derive ServiceTemplate and ServiceTemplateInternal models to build off of. Is that kind of what you were thinking?

My concern is how disruptive this could be and I don't think the current change prevents us from doing the subclassing work in the future.

Thoughts?

bdunne · 2018-07-17T17:58:54Z

I didn't realize this was being backported 😞 Yes, that's what I was thinking with the subclassing, inject an internal class

bzwei · 2018-07-18T21:01:57Z

cc @hsong-rh

gmcculloug

@bzwei Can you turn this into a refactoring that introduces the SERVICE_TYPE constants and remove the "internal" additions as we will be implementing that different in #17748

Remove "unknown" as one of the allowed values for service_type

miq-bot · 2018-07-26T19:32:12Z

Checked commit bzwei@cb706b8 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 0 offenses detected
Everything looks fine. 🏆

gmcculloug · 2018-07-26T19:42:09Z

spec/models/service_template_spec.rb

@@ -240,13 +251,19 @@
    end

    it "with service_type of unknown" do


@bzwei The service_type "unknown" is no longer a concern here. Should this test be removed?

The code still accept any string to service_type. So the test here is ok although not important.

gmcculloug · 2018-07-26T19:42:24Z

spec/models/service_template_spec.rb

@@ -255,11 +272,17 @@
    end

    it "with service_type of unknown" do


bzwei · 2018-07-26T20:48:44Z

@miq-bot remove_label gaprindashvili/yes, bug, transformation
@miq-bot add_label technical_debt

miq-bot · 2018-07-26T20:49:26Z

@bzwei Cannot apply the following label because they are not recognized: technical_debt

This is to exclude Migration Plan Service Templates from Catalogs Explorer. Backend changes to support this are in ManageIQ/manageiq#17681 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1594408

h-kataria · 2018-07-27T23:44:06Z

@bzwei @gmcculloug can you please let me know how does this PR affect changes in ManageIQ/manageiq-ui-classic#4274 , now that public_service_templates scope has been removed from the core PR, changes in UI PR no longer work. Please suggest.

gmcculloug · 2018-07-28T14:19:39Z

@h-kataria Thanks for pointing that out. The public_service_templates approach has not changed but the code moved to PR #17748.

This is to exclude Migration Plan Service Templates from Catalogs Explorer. Backend changes to support this are in ManageIQ/manageiq#17681 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1594408

miq-bot added the wip label Jul 9, 2018

bzwei force-pushed the v2v_internal_type branch 3 times, most recently from a0563de to d4c432a Compare July 10, 2018 15:35

miq-bot assigned gmcculloug Jul 10, 2018

bzwei force-pushed the v2v_internal_type branch from d4c432a to a4654a1 Compare July 10, 2018 16:10

gmcculloug reviewed Jul 10, 2018

View reviewed changes

h-kataria mentioned this pull request Jul 10, 2018

Changes to exclude service templates marked as service_type = internal ManageIQ/manageiq-ui-classic#4274

Merged

bzwei force-pushed the v2v_internal_type branch 3 times, most recently from 68cfb06 to b693848 Compare July 10, 2018 19:02

gmcculloug reviewed Jul 11, 2018

View reviewed changes

bzwei force-pushed the v2v_internal_type branch from b693848 to b0e6d63 Compare July 11, 2018 19:04

bzwei changed the title ~~[WIP] set internal service_type for transformation plan~~ Set internal service_type for transformation plan Jul 11, 2018

miq-bot removed the wip label Jul 11, 2018

gmcculloug requested a review from Fryguy July 11, 2018 20:34

gmcculloug added bug gaprindashvili/yes v2v labels Jul 11, 2018

gmcculloug reviewed Jul 16, 2018

View reviewed changes

bdunne reviewed Jul 16, 2018

View reviewed changes

bzwei force-pushed the v2v_internal_type branch from 81a7835 to d236146 Compare July 16, 2018 18:39

bzwei changed the title ~~Set internal service_type for transformation plan~~ [WIP] Set internal service_type for transformation plan Jul 19, 2018

miq-bot added the wip label Jul 19, 2018

hsong-rh mentioned this pull request Jul 19, 2018

Add internal column to ServiceTemplate table ManageIQ/manageiq-schema#238

Merged

gmcculloug suggested changes Jul 24, 2018

View reviewed changes

bzwei changed the title ~~[WIP] Set internal service_type for transformation plan~~ [WIP] Refactor service_type to use constants Jul 24, 2018

bzwei force-pushed the v2v_internal_type branch from d236146 to c724bbe Compare July 26, 2018 15:44

Define constants used by service_type in service_templates

cb706b8

Remove "unknown" as one of the allowed values for service_type

bzwei force-pushed the v2v_internal_type branch from c724bbe to cb706b8 Compare July 26, 2018 19:24

bzwei changed the title ~~[WIP] Refactor service_type to use constants~~ Refactor service_type to use constants Jul 26, 2018

miq-bot removed the wip label Jul 26, 2018

gmcculloug reviewed Jul 26, 2018

View reviewed changes

miq-bot removed gaprindashvili/yes bug v2v labels Jul 26, 2018

gmcculloug added refactoring technical debt labels Jul 26, 2018

gmcculloug approved these changes Jul 26, 2018

View reviewed changes

gmcculloug merged commit 1893e95 into ManageIQ:master Jul 27, 2018

gmcculloug added this to the Sprint 91 Ending Jul 30, 2018 milestone Jul 27, 2018

bzwei deleted the v2v_internal_type branch July 27, 2018 13:35

mkanoor mentioned this pull request Jul 31, 2018

Removed unknown service type test ManageIQ/manageiq-automation_engine#203

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor service_type to use constants #17681

Refactor service_type to use constants #17681

bzwei commented Jul 9, 2018 •

edited

Loading

bzwei commented Jul 10, 2018

gmcculloug Jul 10, 2018

gmcculloug Jul 10, 2018

gmcculloug Jul 11, 2018

gmcculloug Jul 16, 2018

bdunne left a comment •

edited

Loading

bdunne Jul 16, 2018

bzwei Jul 16, 2018

bdunne Jul 16, 2018

bdunne Jul 16, 2018

bzwei Jul 16, 2018

bdunne Jul 16, 2018

bzwei Jul 16, 2018

gmcculloug commented Jul 17, 2018

bdunne commented Jul 17, 2018

bzwei commented Jul 18, 2018

gmcculloug left a comment

miq-bot commented Jul 26, 2018

gmcculloug Jul 26, 2018

bzwei Jul 26, 2018

gmcculloug Jul 26, 2018

bzwei commented Jul 26, 2018

miq-bot commented Jul 26, 2018

h-kataria commented Jul 27, 2018

gmcculloug commented Jul 28, 2018

		@@ -207,36 +213,35 @@ def create_service(service_task, parent_svc = nil)
		end

		def set_service_type

		@@ -240,13 +251,19 @@
		end

		it "with service_type of unknown" do

		@@ -255,11 +272,17 @@
		end

		it "with service_type of unknown" do

Refactor service_type to use constants #17681

Refactor service_type to use constants #17681

Conversation

bzwei commented Jul 9, 2018 • edited Loading

bzwei commented Jul 10, 2018

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

bdunne left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmcculloug commented Jul 17, 2018

bdunne commented Jul 17, 2018

bzwei commented Jul 18, 2018

gmcculloug left a comment

Choose a reason for hiding this comment

miq-bot commented Jul 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzwei commented Jul 26, 2018

miq-bot commented Jul 26, 2018

h-kataria commented Jul 27, 2018

gmcculloug commented Jul 28, 2018

bzwei commented Jul 9, 2018 •

edited

Loading

bdunne left a comment •

edited

Loading