-
Notifications
You must be signed in to change notification settings - Fork 41
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
Remove region support #52
Conversation
a747e3b
to
f45535c
Compare
Looks like there are some specs around regions here: manageiq-providers-google/spec/models/manageiq/providers/google/cloud_manager_spec.rb Line 52 in 8288913
|
@@ -16,7 +16,6 @@ class ManageIQ::Providers::Google::CloudManager < ManageIQ::Providers::CloudMana | |||
include ManageIQ::Providers::Google::ManagerMixin | |||
|
|||
supports :provisioning | |||
supports :regions | |||
|
|||
before_create :ensure_managers | |||
before_update :ensure_managers_zone_and_provider_region |
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.
@tumido - is this needed?
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.
@bronaghs Why should we keep it there? GCE has currently no usecase for regions at all.
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.
Im referring to line 21, Im implying it looks like it isnt.
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.
Ah, I see, sorry.
Well, this will keep the manager zones in sync and I assume we want that. Also as a sideeffect it would sync the provider_region, sure. Though if it's already nil
, it's nil
for both managers and nothing really happens.
The other way would be to define a new before_update
rule, which would sync only the zone. And also new before_create
rule, since :ensure_managers
uses :ensure_managers_zone_and_provider_region
internally. I just thing leaving it there is harmless. What do you think?
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.
@tumido - Given that these methods have such short implementations, I think it would be worthwhile to override them to keep things clean.
Ok, the spec is pointless (the region part). GCE spans across all regions so there's no point in placing it in one region specifically and checking if it works. It does work, cause there's a restriction for it, but such change has no real impact on provider. I'll remove that from the spec, thanks for pointing that out 👍 |
f45535c
to
97c6488
Compare
Looks like region should be removed from here: manageiq-providers-google/spec/models/manageiq/providers/google/cloud_manager/refresher_spec.rb Line 5 in 78f85fc
|
Good catch with the Factory, I'll remove that. I'll also remove it from core factories at: Sounds good? |
sounds good @tumido |
97c6488
to
630418c
Compare
Checked commit tumido@630418c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
LGTM 👍 |
Remove region support (cherry picked from commit e91c9f6) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1552305
Gaprindashvili backport details:
|
Remove regions completely from GCE for now. They were used only for
ems
descriptions - incorrectly, since the provider operates across regions.Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1433062
Related: ManageIQ/manageiq#17088
@miq-bot add_label bug, gaprindashvili/yes, refactoring