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.

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 the replication exclude tables #5546

Merged
merged 1 commit into from
May 10, 2019

Conversation

carbonin
Copy link
Member

@carbonin carbonin commented May 7, 2019

With the new backend replication technology, making a new set of
excluded tables take effect requires action on both the remote and
global regions. Because of this the user experience would be much
worse for a feature that is not really necessary.

Requires ManageIQ/manageiq#18686

@miq-bot add_label wip, hammer/no, pending core


Before:
image

After:
image

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
With the new backend replication technology, making a new set of
excluded tables take effect requires action on both the remote and
global regions. Because of this the user experience would be much
worse for a feature that is not really necessary.
@carbonin carbonin force-pushed the remove_replication_excludes branch from 7baac0a to 7f0ffc7 Compare May 7, 2019 19:52
@miq-bot
Copy link
Member

miq-bot commented May 7, 2019

Checked commit carbonin@7f0ffc7 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

**

  • 💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:
warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.6-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

@carbonin
Copy link
Member Author

carbonin commented May 9, 2019

I'm thinking that whatever order we merge these two PRs in will likely break the UI specs... 😟

@martinpovolny wondering who would be able to review this one so that we can get it in rather quickly either just before or just after ManageIQ/manageiq#18686 is merged?

@carbonin carbonin changed the title [WIP] Remove the replication exclude tables Remove the replication exclude tables May 9, 2019
@miq-bot miq-bot removed the wip label May 9, 2019
@carbonin
Copy link
Member Author

@himdel Can you point out who should review this? Thanks!

@himdel
Copy link
Contributor

himdel commented May 10, 2019

I guess that's me ;) already looking...

@himdel
Copy link
Contributor

himdel commented May 10, 2019

This looks fine, the field was only available for remote, now it's gone, not seeing any remains of that, saving was updated... 👍

Tested in UI, the saveable logic is also behaving correctly 👍

Also, one less codemirror to worry about 👍 :)

(It looks like we could merge the UI part now without breaking anything, is that right?)


(One thing I do notice, is that saving outputs a Replication configuration save initiated. Check status of task "" on MyTasks screen flash message, where the task name? is always empty. But that's a separate issue, also present on master.)

@carbonin
Copy link
Member Author

@miq-bot add_label changelog/yes

@himdel
Copy link
Contributor

himdel commented May 10, 2019

Aand sounds like merging this one won't break anything after all,
while the other ordering might.

Merging now :)

@himdel himdel merged commit 5a06adb into ManageIQ:master May 10, 2019
@himdel himdel self-assigned this May 10, 2019
@himdel himdel added this to the Sprint 111 Ending May 13, 2019 milestone May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants