Update provider_region to nil for Google provider #184

ZitaNemeckova · 2018-04-05T14:29:42Z

Fixes UI failure after ManageIQ/manageiq-providers-google#52 .
To reproduce:
Make sure you have ManageIQ::Providers::Google::CloudManager or ManageIQ::Providers::Google::NetworkManager with provider_region not set to nil.
Go to summary page of said provider.
Before:

After:

With both providers not having description method but having historically set provider_region following lines are problem solved by removing values from provider_region.
https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/helpers/ems_cloud_helper/textual_summary.rb#L42
https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/helpers/ems_network_helper/textual_summary.rb#L40

@miq-bot add_label wip, bug, blocker

https://bugzilla.redhat.com/show_bug.cgi?id=1563600

Fix for Gaprindashvili: ManageIQ/manageiq-ui-classic#3735

More info

…anager Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1563600

ZitaNemeckova · 2018-04-06T09:33:38Z

@miq-bot remove_label wip

lpichler

looks good but I left here some suggestions ⬆️

lpichler · 2018-04-06T10:38:17Z

db/migrate/20180405145642_remove_provider_region_for_google_provider.rb

+
+  def up
+    say_with_time("Removing provider_region from ManageIQ::Providers::Google::CloudManager") do
+      ExtManagementSystem.where(:type => 'ManageIQ::Providers::Google::CloudManager').each do |provider|


@ZitaNemeckova
I think that we do it in one query.

ExtManagementSystem.where(:type => 'ManageIQ::Providers::Google::CloudManager').update_all(:provider_region => nil)

@lpichler I did but miq-bot complained about update_all. I don't mind changing it back if miq-bot's complain isn't important.

lpichler · 2018-04-06T10:51:26Z

spec/migrations/20180405145642_remove_provider_region_for_google_provider_spec.rb

+      ems_stub.create!(:type            => 'ManageIQ::Providers::Vmware::CloudManager',
+                       :provider_region => 42)
+
+      migrate


@ZitaNemeckova

I would join your its to one it with expect( because it would be nice to keep one it case for one migrating process and adapt 'it' message.

I would also prefer creating object in lets although it is not needed in this case if there will be only one it.

move migrate command to the it and remove before block.

What do you think about this suggestions ?

tumido

Looks good 👍

Just a note for other reviewers, this bug was introduced by me in a fix for https://bugzilla.redhat.com/show_bug.cgi?id=1433062

The resolution described there explains why such change is necessary and also why here's no down migration present.

tumido · 2018-04-06T14:32:38Z

spec/migrations/20180405145642_remove_provider_region_for_google_provider_spec.rb

+  migration_context :up do
+    let(:ems_stub) { migration_stub :ExtManagementSystem }
+
+    ems_stub.create!(:type            => 'ManageIQ::Providers::Google::CloudManager',


Please wrap these calls into before block

lpichler

Thanks @ZitaNemeckova !

h-kataria · 2018-04-06T14:53:47Z

@ZitaNemeckova just a note: this PR contains changes to schema, so can not be backported to Gaprindashvili or Fine branches, will need a fix in the code to handle the issue on those branches.

lpichler · 2018-04-06T14:56:53Z

@miq-bot assign @Fryguy

tumido · 2018-04-06T15:02:20Z

@h-kataria, I don't think it changes the schema, only data for one (or two possibly) specific emses

@ZitaNemeckova, I wonder, is this change needed only for Google::CloudManager? Don't we need this for Google::NetworkManager manager as well?

…nager as well Added it to migration and specs

miq-bot · 2018-04-09T07:32:27Z

Checked commits ZitaNemeckova/manageiq-schema@ad28f50~...5b0534d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 2 offenses detected

db/migrate/20180405145642_remove_provider_region_for_google_provider.rb

❗ - Line 11, Col 89 - Rails/SkipsModelValidations - Avoid using update_all because it skips validations.
❗ - Line 8, Col 87 - Rails/SkipsModelValidations - Avoid using update_all because it skips validations.

tumido

LGTM 👍

h-kataria · 2018-04-09T18:20:27Z

@Fryguy @gtanzillo please review/merge

gtanzillo · 2018-04-10T13:19:56Z

@Fryguy You ok with merging this one?

Fryguy · 2018-04-10T14:40:06Z

Are you expecting this to be backported? We can't backport migrations.

ZitaNemeckova · 2018-04-10T15:24:12Z

No, Gaprindashvili will be fixed by UI workaround.

h-kataria · 2018-04-10T15:29:45Z

@Fryguy UI PR with workaround for G branch ManageIQ/manageiq-ui-classic#3735

Fryguy

Looks good, but need an answer re:backporting. Is ManageIQ/manageiq-ui-classic#3735 the alternative for gaprindashvili?

Fryguy · 2018-04-10T15:39:01Z

And there's the answer...thanks GitHub for not refreshing ;)

miq-bot changed the title ~~Update provider_region to nil for Google provider~~ [WIP] Update provider_region to nil for Google provider Apr 5, 2018

miq-bot added blocker bug wip labels Apr 5, 2018

ZitaNemeckova force-pushed the update_provider_region branch 2 times, most recently from be8a807 to 0d33413 Compare April 6, 2018 08:57

Update provider_region to nil for ManageIQ::Providers::Google::CloudM…

ad28f50

…anager Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1563600

ZitaNemeckova force-pushed the update_provider_region branch 2 times, most recently from eeedc8e to aa80005 Compare April 6, 2018 09:18

miq-bot changed the title ~~[WIP] Update provider_region to nil for Google provider~~ Update provider_region to nil for Google provider Apr 6, 2018

miq-bot removed the wip label Apr 6, 2018

lpichler suggested changes Apr 6, 2018

View reviewed changes

ZitaNemeckova force-pushed the update_provider_region branch from 55ed535 to 2dce0a3 Compare April 6, 2018 14:31

tumido reviewed Apr 6, 2018

View reviewed changes

lpichler approved these changes Apr 6, 2018

View reviewed changes

ZitaNemeckova force-pushed the update_provider_region branch from 2dce0a3 to 95e879c Compare April 6, 2018 14:46

ZitaNemeckova added 2 commits April 6, 2018 16:49

Add check provider_region is not set to nil for non-Google provider

7eb7aeb

Fix specs according to review

6ec0ed8

ZitaNemeckova force-pushed the update_provider_region branch from 95e879c to 6ec0ed8 Compare April 6, 2018 14:49

miq-bot assigned Fryguy Apr 6, 2018

Set provider_region to nil for ManageIQ::Providers::Google::NetworkMa…

5b0534d

…nager as well Added it to migration and specs

ZitaNemeckova mentioned this pull request Apr 9, 2018

[GAPRINDASHVILI] Check @record has description method before using provider_region row ManageIQ/manageiq-ui-classic#3735

Merged

tumido approved these changes Apr 9, 2018

View reviewed changes

gtanzillo approved these changes Apr 9, 2018

View reviewed changes

Fryguy approved these changes Apr 10, 2018

View reviewed changes

Fryguy merged commit f49d5d5 into ManageIQ:master Apr 10, 2018

Fryguy added this to the Sprint 84 Ending Apr 23, 2018 milestone Apr 10, 2018

Fryguy removed the blocker label Apr 10, 2018

ZitaNemeckova deleted the update_provider_region branch November 14, 2019 11:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update provider_region to nil for Google provider #184

Update provider_region to nil for Google provider #184

ZitaNemeckova commented Apr 5, 2018 •

edited

Loading

ZitaNemeckova commented Apr 6, 2018

lpichler left a comment

lpichler Apr 6, 2018

ZitaNemeckova Apr 6, 2018

lpichler Apr 6, 2018

tumido left a comment

tumido Apr 6, 2018

lpichler left a comment

h-kataria commented Apr 6, 2018

lpichler commented Apr 6, 2018

tumido commented Apr 6, 2018

miq-bot commented Apr 9, 2018

tumido left a comment

h-kataria commented Apr 9, 2018

gtanzillo commented Apr 10, 2018

Fryguy commented Apr 10, 2018

ZitaNemeckova commented Apr 10, 2018

h-kataria commented Apr 10, 2018

Fryguy left a comment

Fryguy commented Apr 10, 2018

Update provider_region to nil for Google provider #184

Update provider_region to nil for Google provider #184

Conversation

ZitaNemeckova commented Apr 5, 2018 • edited Loading

ZitaNemeckova commented Apr 6, 2018

lpichler left a comment

Choose a reason for hiding this comment

lpichler Apr 6, 2018

Choose a reason for hiding this comment

ZitaNemeckova Apr 6, 2018

Choose a reason for hiding this comment

lpichler Apr 6, 2018

Choose a reason for hiding this comment

tumido left a comment

Choose a reason for hiding this comment

tumido Apr 6, 2018

Choose a reason for hiding this comment

lpichler left a comment

Choose a reason for hiding this comment

h-kataria commented Apr 6, 2018

lpichler commented Apr 6, 2018

tumido commented Apr 6, 2018

miq-bot commented Apr 9, 2018

tumido left a comment

Choose a reason for hiding this comment

h-kataria commented Apr 9, 2018

gtanzillo commented Apr 10, 2018

Fryguy commented Apr 10, 2018

ZitaNemeckova commented Apr 10, 2018

h-kataria commented Apr 10, 2018

Fryguy left a comment

Choose a reason for hiding this comment

Fryguy commented Apr 10, 2018

ZitaNemeckova commented Apr 5, 2018 •

edited

Loading