Fixed error with replication setup when default exclude list used #17999
Conversation
| @@ -158,6 +158,12 @@ | |||
| expect(described_class).to receive(:refresh_excludes).with(YAML.safe_load(tables)) | |||
| described_class.save_remote_region(tables) | |||
| end | |||
|
|
|||
| it "does not updates list of tables to be excluded from replication if passed paramere is empty" do | |||
There was a problem hiding this comment.
Is it okay to have an empty exclusion_list? I think there are some tables we are counting on never replicating (MiqDatabase).
There was a problem hiding this comment.
Yeah, we have this but it's not being used very thoroughly for application-side tables (but rather for AR and other system-type tables).
There was a problem hiding this comment.
There are probably lots of tables that would cause problems if they were replicated, but we have never tried to enumerate them in any serious way.
There was a problem hiding this comment.
Also, this has some typos s/updates/update/ s/paramere/parameter/
lib/miq_pglogical.rb
Outdated
| @@ -145,7 +145,7 @@ def self.default_excludes | |||
|
|
|||
| def self.save_remote_region(exclusion_list) | |||
| MiqRegion.replication_type = :remote | |||
| refresh_excludes(YAML.safe_load(exclusion_list)) | |||
| refresh_excludes(YAML.safe_load(exclusion_list)) unless exclusion_list.empty? | |||
There was a problem hiding this comment.
Why is this exclude list empty?
There was a problem hiding this comment.
Or rather why would it ever be empty unless the user specifically wanted to exclude nothing? (which as @bdunne noted will likely not go well)
There was a problem hiding this comment.
Okay, after going back over the previous two PRs it looks like this condition was previously preventing us from calling the refresh_excludes if the exclude list parameter was empty.
I'm not particularly happy that this UI-side concern (dealing with an empty list that shouldn't be treated as an empty list) is being pushed into this method, but I'm not sure how else this can be solved with the new way this is working.
Can you add a comment that says that this is here because the UI passes an empty array for "no changes" so we can rely on the refresh_excludes call which is run as a part of replication_type=?
|
After looking into this for a bit, it seems like your previous changes would have failed unconditionally @yrudman. In the future, when making complex changes like this, please test them before making it sound like the PRs are ready to be merged. |
|
Checked commits yrudman/manageiq@154e779~...48e92b0 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
Fixing error when when default list of tables to exclude from replication used during remote region. This bug was introduced in #17956
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1630517
@miq-bot add-label bug