Skip to content
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.

Sign up for GitHub

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

Fixed error with replication setup when default exclude list used #17999

Merged
merged 4 commits into from
Sep 20, 2018
Merged

Fixed error with replication setup when default exclude list used #17999

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
154e779
do not refresh list of tabled to be excluded from replication if para…
yrudman Sep 19, 2018
00df0f2
rspec - parameter passed to is empty
yrudman Sep 19, 2018
6c356d2
added comments clarifying .save_remote_region
yrudman Sep 20, 2018
48e92b0
fixed spelling
yrudman Sep 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/miq_pglogical.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this exclude list empty?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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=?

end

def self.save_global_region(subscriptions_to_save, subscriptions_to_remove)
Expand Down
6 changes: 6 additions & 0 deletions spec/replication/util/miq_pglogical_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

@bdunne bdunne Sep 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it okay to have an empty exclusion_list? I think there are some tables we are counting on never replicating (MiqDatabase).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this has some typos s/updates/update/ s/paramere/parameter/

allow(MiqRegion).to receive(:replication_type=)
expect(described_class).not_to receive(:refresh_excludes)
described_class.save_remote_region("")
end
end

describe ".save_global_region" do
Expand Down