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

Update model to use the customization_scripts table for LXCA config patterns #16036

Merged
merged 3 commits into from Oct 9, 2017

Conversation

skovic
Copy link

@skovic skovic commented Sep 25, 2017

Recently, a new table named configuration_templates was created to hold data related to configuration patterns. Based on feedback received from Red Hat, this PR updates the model to use an existing table, customization_scripts, to hold configuration pattern data instead.

@skovic
Copy link
Author

skovic commented Sep 25, 2017

Schema changes are in ManageIQ/manageiq-schema#75

@skovic
Copy link
Author

skovic commented Sep 25, 2017

@miq-bot assign @blomquisg

Copy link
Member

@blomquisg blomquisg left a comment

Choose a reason for hiding this comment

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

A couple minor changes. There is a test failure, but I'm not sure I see the overlap with this code. It's possible that cleaning up the things I mentioned will fix it, but I doubt it.

@@ -3,4 +3,5 @@ class CustomizationScript < ApplicationRecord

acts_as_miq_taggable
belongs_to :provisioning_manager
Copy link
Member

Choose a reason for hiding this comment

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

Ah, this line will have to be removed with the next line added. It might break some tests that have to be cleaned up. But, the provisioning_manager and the ext_mangement_system are actually really the same thing (both pointing to ext_management_systems table).

@@ -52,18 +52,18 @@ def save_physical_servers_inventory(ems, hashes, target = nil)
store_ids_for_new_records(ems.physical_servers, hashes, :ems_ref)
end

def save_configuration_templates_inventory(ems, hashes, target = nil)
def save_customization_scripts(ems, hashes, target = nil)
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 should have _inventory on the end. Otherwise I don't think it will be called.

@skovic
Copy link
Author

skovic commented Sep 27, 2017

@blomquisg I've made the changes you recommended, and the tests are still failing. I think it's because the schema changes haven't been merged yet.

@Fryguy
Copy link
Member

Fryguy commented Oct 2, 2017

@bdunne Please review.

@blomquisg blomquisg dismissed their stale review October 3, 2017 18:11

Looks better now

@blomquisg blomquisg requested a review from bdunne October 3, 2017 18:11
@@ -2,5 +2,5 @@ class CustomizationScript < ApplicationRecord
include NewWithTypeStiMixin

acts_as_miq_taggable
belongs_to :provisioning_manager
belongs_to :ext_management_system, :foreign_key => "manager_id"
Copy link
Member

Choose a reason for hiding this comment

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

Can this (and the corresponding migration) be split into separate PRs? They're not really related to the rest of the changes here.

Also, would you mind deprecating the old belongs_to with:

deprecate_belongs_to(:provisioning_manager, :ext_management_system)

Copy link
Author

Choose a reason for hiding this comment

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

@bdunne I tested this change, and it looks like the deprecate is causing a problem. I am getting the following error in the UI:
undefined method `deprecate_belongs_to' for #Class:0x007f3ac1a98ff0 Did you mean? deprecate_constant [ems_physical_infra/show_list]

Copy link
Member

Choose a reason for hiding this comment

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

Oh you'll also need to include DeprecationMixin in this class in order to use deprecate_belongs_to

@@ -59,7 +59,7 @@ def self.supported_types_and_descriptions_hash
has_many :customization_specs, :foreign_key => "ems_id", :dependent => :destroy, :inverse_of => :ext_management_system
has_many :storage_profiles, :foreign_key => "ems_id", :dependent => :destroy, :inverse_of => :ext_management_system
has_many :physical_servers, :foreign_key => "ems_id", :dependent => :destroy, :inverse_of => :ext_management_system
has_many :configuration_templates, :foreign_key => "ems_id", :dependent => :destroy, :inverse_of => :ext_management_system
has_many :customization_scripts, :foreign_key => "manager_id", :dependent => :destroy, :inverse_of => :ext_management_system
Copy link
Author

Choose a reason for hiding this comment

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

@bdunne Is it safe to leave this here? Or should it also be moved to the other PR since it uses the renamed "manager_id" field?

Copy link
Member

Choose a reason for hiding this comment

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

This is fine.

@miq-bot
Copy link
Member

miq-bot commented Oct 4, 2017

Checked commits skovic/manageiq@5a27d67~...dda559a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@skovic
Copy link
Author

skovic commented Oct 4, 2017

@bdunne I moved the customization_script model changes to PR #16119 . I think I have everything separated now as you requested. Please let me know if I missed anything. Thanks

@skovic
Copy link
Author

skovic commented Oct 4, 2017

@bdunne Looks like the travis tests are failing now because of the changes we moved out of this PR.

@skovic skovic closed this Oct 5, 2017
@skovic skovic reopened this Oct 5, 2017
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.

👍 LGTM, just waiting on the schema PR

@@ -59,7 +59,7 @@ def self.supported_types_and_descriptions_hash
has_many :customization_specs, :foreign_key => "ems_id", :dependent => :destroy, :inverse_of => :ext_management_system
has_many :storage_profiles, :foreign_key => "ems_id", :dependent => :destroy, :inverse_of => :ext_management_system
has_many :physical_servers, :foreign_key => "ems_id", :dependent => :destroy, :inverse_of => :ext_management_system
has_many :configuration_templates, :foreign_key => "ems_id", :dependent => :destroy, :inverse_of => :ext_management_system
has_many :customization_scripts, :foreign_key => "manager_id", :dependent => :destroy, :inverse_of => :ext_management_system
Copy link
Member

Choose a reason for hiding this comment

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

This is fine.

@Fryguy Fryguy merged commit d32b663 into ManageIQ:master Oct 9, 2017
@Fryguy Fryguy added this to the Sprint 71 Ending Oct 16, 2017 milestone Oct 9, 2017
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.

None yet

5 participants