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

Ensure EMS relationships are established #103

Closed
wants to merge 7 commits into from
Closed

Ensure EMS relationships are established #103

wants to merge 7 commits into from

Conversation

djberg96
Copy link
Contributor

This addresses an issue, originally on the Amazon provider side, where the EMS relationships were not established when a StorageManager was added.

Originally brought to our attention via https://bugzilla.redhat.com/show_bug.cgi?id=1480629, but 5.7 -> 5.8 will be handled with a self.seed method.

@Fryguy
Copy link
Member

Fryguy commented Oct 24, 2017

The migrations will need a stub with a copy of the ensure_managers method. As migrations are a point-in-time snapshot, we cannot have changes to the underlying ensure_managers method from affecting old migrations. Thus, we need a copy.

@@ -0,0 +1,5 @@
class EnsureCloudManagers < ActiveRecord::Migration[5.0]
def change
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be up, because it is not a reversible operation.

@bronaghs
Copy link

@djberg96 - bump ^^
Migrations have to be merged by Monday 10/30

@djberg96
Copy link
Contributor Author

@Fryguy I don't know if I did it the "expected" way, but please take a look and see what you think. I can squash the commits if you're happy with it.

@Fryguy
Copy link
Member

Fryguy commented Oct 27, 2017

all data migrations require specs as well. I think this is passing by virtue of it not doing anything (i.e. there is no data in the database, so when it queries it has no data).


class ManageIQ
class Providers
class CloudManager < BaseManager
Copy link
Member

Choose a reason for hiding this comment

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

This should be modifying the stub model created above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fryguy Sorry, I don't follow.

class EnsureCloudManagers < ActiveRecord::Migration[5.0]
class ExtManagementSystem < ActiveRecord::Base; end
class BaseManager < ExtManagementSystem; end
class Settings < ActiveRecord::Base; end
Copy link
Member

Choose a reason for hiding this comment

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

I don;t know why Settings is needed...Settings is not even an ActiveRecord model in the manageiq app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Settings (originally referenced as ::Settings) is referred to within the ensure_managers method. I've modified it so that it's not a subclass of AR any more.

@@ -0,0 +1,47 @@
class EnsureCloudManagers < ActiveRecord::Migration[5.0]
class ExtManagementSystem < ActiveRecord::Base; end
Copy link
Member

Choose a reason for hiding this comment

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

This needs the

self.inheritance_column = :_type_disabled # disable STI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@miq-bot
Copy link
Member

miq-bot commented Oct 27, 2017

Checked commits https://github.com/djberg96/manageiq-schema/compare/ac5aa05ea0beae70bb680617f3f4af576319ff2d~...361226b3bbc516c599ae97009b12cb6c2834c337 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 1 offense detected

db/migrate/20171023170841_ensure_cloud_managers.rb

@chessbyte
Copy link
Member

Replaced by #117

@chessbyte chessbyte closed this Oct 30, 2017
@miq-bot
Copy link
Member

miq-bot commented Oct 30, 2017

This pull request is not mergeable. Please rebase and repush.

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.

5 participants