Remove all VMware MKS console-related records from SettingSchanges #166
Conversation
| let(:server) { server_stub.create } | ||
|
|
||
| before do | ||
| settings_stub.create(:resource => server, :key => '/server/remote_console_type', :value => 'MKS') |
There was a problem hiding this comment.
This can be inside the it since there's only 1 test.
| @@ -0,0 +1,7 @@ | |||
| class DropWebmks < ActiveRecord::Migration[5.0] | |||
| class SettingsChange < ApplicationRecord; end | |||
There was a problem hiding this comment.
I think we want ActiveRecord::Base to be the parent class in this case since migrations shouldn't be using any models. cc @Fryguy
| class SettingsChange < ApplicationRecord; end | ||
|
|
||
| def up | ||
| SettingsChange.where(:resource_type => 'MiqServer', :key => '/server/remote_console_type', :value => 'MKS').delete_all |
There was a problem hiding this comment.
We don't need the :resource_type. Removing that from the scope would probably be a good idea in case someone added it to a zone settings.
skateman
force-pushed
the
drop-mks
branch
3 times, most recently
from
4a89df3
to
a670999
Compare
| let(:server) { server_stub.create } | ||
|
|
||
| it 'resets the remote_console_type to default' do | ||
| settings_stub.create(:resource => server, :key => '/server/remote_console_type', :value => 'MKS') |
There was a problem hiding this comment.
Now the tests are failing because you either need to specify :resource_type and :resource_id or define the relation in the class defined above.
skateman
force-pushed
the
drop-mks
branch
2 times, most recently
from
2663b83
to
3c73b2f
Compare
| class SettingsChange < ActiveRecord::Base; end | ||
|
|
||
| def up | ||
| SettingsChange.where(:key => '/server/remote_console_type', :value => 'MKS').delete_all |
There was a problem hiding this comment.
you can wrap it with say_with_time
|
Migrations cannot be backported. If these records are in the database is there a problem with that? |
| settings_stub.create(:resource_id => server.id, :resource_type => server.class, :key => '/server/remote_console_type', :value => 'MKS') | ||
| migrate | ||
|
|
||
| expect(settings_stub.where(:key => '/server/remote_console_type', :value => 'MKS').count).to be_zero |
|
Code-wise looks good to me, but I want to understand the backport strategy before merging. |
|
@Fryguy well we had a discussion with @bdunne at the original euwe-only PR, check it out. |
|
@Fryguy I moved out the schema code from the #16909. After a little bit of thinking I don't think this is really necessary but it would not do any harm, so your call to merge or close. |
|
Checked commit skateman@c7f75ab with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
This feature has been dropped in fine and later, however, the migration was missing. There's also a PR for EUWE: ManageIQ/manageiq#16909
https://bugzilla.redhat.com/show_bug.cgi?id=1513592
@miq-bot assign @bdunne
@miq-bot add_label fine/yes, gaprindashvili/yes