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

Migrate MiddlewareServer to MiddlewareServerWildfly and MiddlewareServerEap #81

Merged
merged 5 commits into from
Oct 23, 2017

Conversation

tumido
Copy link
Member

@tumido tumido commented Sep 29, 2017

Sub-classing MiddlewareServer into more specific classes for Wildfly and EAP

Related to: ManageIQ/manageiq-providers-hawkular#60
Resolves: https://issues.jboss.org/browse/HAWKULAR-1248

@tumido
Copy link
Member Author

tumido commented Sep 29, 2017

@miq-bot add_label enhancement
cc @cfcosta please review

@@ -0,0 +1,33 @@
class MigrateMiddlewareServerToWildflyAndEap < ActiveRecord::Migration[5.0]
class MiddlewareServer < ApplicationRecord
Copy link
Member

Choose a reason for hiding this comment

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

Be sure to add self.inheritance_column = :_type_disabled for this stub.

Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

Specs are required for data migrations.

@Fryguy Fryguy added the data label Sep 29, 2017
@@ -0,0 +1,33 @@
class MigrateMiddlewareServerToWildflyAndEap < ActiveRecord::Migration[5.0]
class MiddlewareServer < ApplicationRecord
Copy link
Contributor

Choose a reason for hiding this comment

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

class MiddlewareServer < ActiveRecord::Base

see details:
#4

@miq-bot
Copy link
Member

miq-bot commented Sep 29, 2017

Checked commits tumido/manageiq-schema@47df2bc~...3f7ce44 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 5 offenses detected

db/migrate/20170928202851_migrate_middleware_server_to_wildfly_and_eap.rb

@Fryguy
Copy link
Member

Fryguy commented Sep 29, 2017

Just want approval from @abonas here and on the dependent PR before merging.

@tumido
Copy link
Member Author

tumido commented Sep 29, 2017

Thanks @Fryguy 👍 :)

@abonas
Copy link
Member

abonas commented Oct 1, 2017

@cfcosta please review.
@tumido please see the comment in ManageIQ/manageiq-providers-hawkular#60 (review)

say_with_time('Migrating middleware_server to middleware_server_wildfly') do
MiddlewareServer
.where("type = ? AND product ~* ?", 'ManageIQ::Providers::Hawkular::MiddlewareManager::MiddlewareServer', 'wildfly')
.update_all(:type => 'ManageIQ::Providers::Hawkular::MiddlewareManager::MiddlewareServerWildfly')
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't the new wildfly/eap class be a subclass/subtype of middleware server class? here it seems it's in hierarchy of middleware_manager, or I am probably missing something

Copy link

Choose a reason for hiding this comment

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

I think they are better included in middleware_manager as well, so that we have proper namespacing for the resources, what do you think?

Another option would be ManageIQ::Providers::Hawkular::WildFlyServer. Any of the two are fine by me.

@abonas
Copy link
Member

abonas commented Oct 15, 2017

@cfcosta please review

@abonas
Copy link
Member

abonas commented Oct 18, 2017

approved

Copy link

@cfcosta cfcosta left a comment

Choose a reason for hiding this comment

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

LGTM


migration_context :down do
it "reverts type for WildFly" do
mw_server = mw_server_stub.create!(:type => "ManageIQ::Providers::Hawkular::MiddlewareManager::MiddlewareServerWildfly")
Copy link

Choose a reason for hiding this comment

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

Both local assignments should be extracted to let, and the migration to be run in a before filter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you please point me to an example? I can't find any other migration which would be using it... Or maybe I just don't understand the suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cfcosta, does it make any difference here to move two assignments from test cases into two lets?

Copy link

Choose a reason for hiding this comment

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

Just checked on other migration tests, it seems like all of them are done the way you did them, so 🚢 it.

@abonas
Copy link
Member

abonas commented Oct 22, 2017

@Fryguy @chessbyte lets get this in b4 the feature ❄️

@Fryguy Fryguy merged commit 1f0820d into ManageIQ:master Oct 23, 2017
@Fryguy Fryguy added this to the Sprint 72 Ending Oct 30, 2017 milestone Oct 23, 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.

6 participants