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

Jump to bottom

Replace conditions with scope #16399

Merged
merged 1 commit into from
Nov 29, 2017
Merged

Replace conditions with scope #16399

merged 1 commit into from
Nov 29, 2017

Conversation

alexander-demicev
Copy link

@alexander-demicev alexander-demicev commented Nov 6, 2017 •
edited by simaishi
Loading

Replace sql conditions with named scopes.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1507916

@alexander-demicev alexander-demicev mentioned this pull request Nov 6, 2017
Merged
@miq-bot miq-bot added the wip label Nov 6, 2017
@chessbyte chessbyte added the core label Nov 6, 2017
lpichler
lpichler reviewed Nov 22, 2017
View reviewed changes
app/models/system_service.rb Outdated
scope :failed_systemd_services, -> { where(:systemd_active => 'failed').or(where(:systemd_sub => 'failed')) }
scope :host_service_group, ->(host_service_group_id) { where(:host_service_group_id => host_service_group_id) }
scope :host_service_group_running_systemd, lambda { |host_service_group_id|
running_systemd_services & host_service_group(host_service_group_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

running_systemd_services.merge(host_service_group(host_service_group_id))

@alexander-demicev alexander-demicev changed the title [WIP] Replace conditions with scope Replace conditions with scope Nov 25, 2017
@miq-bot miq-bot removed the wip label Nov 25, 2017
@miq-bot
Copy link
Member

miq-bot commented Nov 27, 2017

Checked commit alexander-demicev@7c377bf with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@alexander-demicev
Copy link
Author

alexander-demicev commented Nov 27, 2017 •
edited
Loading

@lpichler @gtanzillo Hi, can I ask for review?

@gtanzillo
Copy link
Member

@alexander-demichev There are callers of those methods in the provider repos. Do you have additional PRs to change the callers?

@alexander-demicev
Copy link
Author

@gtanzillo I have 2 related PR's ManageIQ/manageiq-providers-openstack#144 ManageIQ/manageiq-ui-classic#2638

lpichler
lpichler approved these changes Nov 29, 2017
View reviewed changes
Copy link
Contributor

@lpichler lpichler left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@gtanzillo gtanzillo added this to the Sprint 75 Ending Dec 11, 2017 milestone Nov 29, 2017
@gtanzillo gtanzillo merged commit 894bbfb into ManageIQ:master Nov 29, 2017
@aufi aufi mentioned this pull request Dec 6, 2017
Merged
@simaishi
Copy link
Contributor

@alexander-demichev @gtanzillo ManageIQ/manageiq-providers-openstack#144 depends on this PR and that's marked as gaprindashvili/yes. Can this PR be gaprindashvili/yes too?

@simaishi
Copy link
Contributor

simaishi commented Jan 4, 2018

@alexander-demichev @gtanzillo ping ^

@simaishi
Copy link
Contributor

simaishi commented Mar 7, 2018

@alexander-demichev @gtanzillo ping ^

@alexander-demichev We also need a BZ if this should go to gaprindashvili branch.

@alexander-demicev alexander-demicev deleted the delete-conditions branch March 7, 2018 14:12
@alexander-demicev
Copy link
Author

@miq-bot add_label gaprindashvili/yes

@alexander-demicev
Copy link
Author

@simaishi hi, there is an issue in github ManageIQ/manageiq-ui-classic#2517 related to this BZ

@simaishi
Copy link
Contributor

simaishi commented Mar 7, 2018

@alexander-demichev Change to z-stream requires a BZ to backport...

@gtanzillo
Copy link
Member

@simaishi I'm 👍 with gaprindashvili/yes on this PR. However, it seems that ManageIQ/manageiq-ui-classic#2517 would need to be resolved first.

@alexander-demicev
Copy link
Author

@gtanzillo @simaishi I closed ManageIQ/manageiq-ui-classic#2517

@simaishi
Copy link
Contributor

simaishi commented Mar 8, 2018

@alexander-demichev still need a BZ!

@simaishi
Copy link
Contributor

simaishi commented Jun 5, 2018

Never mind... found BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1507916

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gtanzillo gtanzillo gtanzillo approved these changes

@lpichler lpichler lpichler approved these changes

Assignees

@gtanzillo gtanzillo

Labels
core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alexander-demicev @miq-bot @gtanzillo @simaishi @lpichler @chessbyte