-
Notifications
You must be signed in to change notification settings - Fork 125
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
Set the switch types for VMware switches #180
Conversation
305f7ef
to
ab49622
Compare
@Fryguy You ok with merging this one? |
|
||
class Host < ActiveRecord::Base | ||
self.inheritance_column = :_type_disabled | ||
has_many :host_switches |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to add :class => "UpdateSwitchTypes::HostSwitch" so that it resolves correctly. Frankly, I'm surprised this doesn't fail in specs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, these relationships aren't used anywhere in this migration. If they're only here for testing I would remove them, as we try to keep models in tests down to the bare essentials. You can just query the table directly on the foreign_key column in the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved Host and HostSwitch over to the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah idk I didn't need to set :class_name
for running the tests or for running the migrations against a live db, just added it to be safe
let(:host_stub) { migration_stub(:Host) } | ||
|
||
migration_context :up do | ||
it "migrates a series of representative rows" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any tests for the non-nil switch type case? ( to show they aren't affected )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 added one here
47acfef
to
e2e649d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but something is wrong with the tests...it's possible the .joins(:hosts) requires the host model to be defined in the migration.
Update existing switches to set their types to either DistributedVirtualSwitch for shared switches or HostVirtualSwitch for non-shared switches.
Checked commit agrare@10edec6 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 db/migrate/20180316164826_update_switch_types.rb
|
Update existing switches to set their types to either
DistributedVirtualSwitch for shared switches or HostVirtualSwitch for
non-shared switches.
Depends: ManageIQ/manageiq-providers-vmware#212