Bug 1546535: include shared external networks in list on router create #17305
Conversation
https://bugzilla.redhat.com/show_bug.cgi?id=1546535 When generating the list of networks for router external gateway network, include shared networks owned by other tenants.
|
Checked commit sseago@bed4a6a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
| @@ -50,6 +50,10 @@ def self.class_by_ems(ext_management_system, _external = false) | |||
| ext_management_system && ext_management_system.class::CloudNetwork | |||
| end | |||
|
|
|||
| def self.tenant_id_clause_format(tenant_ids) | |||
| ["((tenants.id IN (?) OR cloud_networks.shared IS TRUE) AND ext_management_systems.tenant_mapping_enabled IS TRUE) OR ext_management_systems.tenant_mapping_enabled IS FALSE OR ext_management_systems.tenant_mapping_enabled IS NULL", tenant_ids] | |||
There was a problem hiding this comment.
Where does tenants come from? I see a belongs_to :cloud_tenant
There was a problem hiding this comment.
@bdunne The reference comes from the included CloudTenancyMixin -- this method overrides tenant_id_clause_format in the mixin to add the part about cloud_networks, but the rest is right from there. As for the actual source of the tenants table in the query mix, it looks like that comes from this reference in cloud_tenant.rb: "has_one :source_tenant, :as => :source, :class_name => 'Tenant' "
There was a problem hiding this comment.
@bdunne Any other issues with the PR?
There was a problem hiding this comment.
Can we write this in AREL? We try to avoid raw sql when possible.
There was a problem hiding this comment.
@Ladas I was trying to keep the implementation as close as possible to the method I'm overriding, which currently uses raw sql. If that gets updated to use AREL, then this should be modified to do the same: https://github.com/ManageIQ/manageiq/blob/master/app/models/mixins/cloud_tenancy_mixin.rb#L16-L18
There was a problem hiding this comment.
Sounds good then, the refactoring should start in the original method. 👍
There was a problem hiding this comment.
In light of @Ladas's comment, can this be merged?
There was a problem hiding this comment.
ping @Ladas @bdunne ^ #17305 (comment)
There was a problem hiding this comment.
I don't have merge rights for this repo, @bdunne ?
|
@miq-bot add_label blocker |
Bug 1546535: include shared external networks in list on router create (cherry picked from commit 30c4dab) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1596248 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1596249
|
Gaprindashvili backport details: |
https://bugzilla.redhat.com/show_bug.cgi?id=1546535
https://bugzilla.redhat.com/show_bug.cgi?id=1594665
When generating the list of networks for router external gateway
network, include shared networks owned by other tenants.