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

Fix alerts based on hourly timer for container entities #16902

Merged
merged 1 commit into from
Feb 15, 2018
Merged

Fix alerts based on hourly timer for container entities #16902

merged 1 commit into from
Feb 15, 2018

Conversation

zeari
Copy link

@zeari zeari commented Jan 29, 2018 •
edited
Loading

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

Alerts driven by hourly timer that evaluate an expression would fail for container entities that had no relationship to Zone. Added ContainerProject, ContainerNode, ContainerReplicator, Container, ContainerGroup.

@cben @moolitayer PTAL
cc @oourfali

@miq-bot add_label bug, providers/containers

zeari
zeari commented Jan 29, 2018
View reviewed changes
app/models/zone.rb
@@ -8,6 +8,7 @@ class Zone < ApplicationRecord

has_many :miq_servers
has_many :ext_management_systems
has_many :container_managers, :class_name => "ManageIQ::Providers::ContainerManager"
Copy link
Author

Choose a reason for hiding this comment

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

I wish we didnt need this and i could just have the lines below as:
has_many :container_nodes, :through => :ext_management_systems
instead.

suggestions?

Copy link
Contributor

@cben cben Jan 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

ActiveRecord ¯_(ツ)_/¯ How I Learnt to Stop Worrying And Just Add :through Associations
(made with https://dev.to/rly)

Copy link
Contributor

@cben cben Jan 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I find your solution pretty neat, actually 👍. But I'm not a rails expert.

zeari
zeari commented Jan 29, 2018
View reviewed changes
app/models/zone.rb Outdated
@@ -19,6 +20,8 @@ class Zone < ApplicationRecord
has_many :vms, :through => :ext_management_systems
has_many :miq_templates, :through => :ext_management_systems
has_many :ems_clusters, :through => :ext_management_systems
has_many :container_nodes, :through => :container_managers
has_many :container_projects, :through => :container_managers
Copy link
Author

Choose a reason for hiding this comment

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

what other container entities make sense here? Pods? Containers?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure anything except EMS and N=ode is possible in UI.

@zeari zeari force-pushed the fix_hourly_event_alerts branch from 4bd7f27 to 3258774 Compare January 29, 2018 14:01
@miq-bot
Copy link
Member

miq-bot commented Jan 29, 2018

Checked commit zeari@3258774 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 13 offenses detected

app/models/zone.rb

spec/models/miq_alert_spec.rb

cben
cben approved these changes Jan 30, 2018
View reviewed changes
Copy link
Contributor

@cben cben left a comment

Choose a reason for hiding this comment

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

LGTM

@zeari
Copy link
Author

zeari commented Feb 8, 2018

@agrare @Ladas Please review

@zeari
Copy link
Author

zeari commented Feb 15, 2018

@agrare @Ladas ping

Ladas
Ladas approved these changes Feb 15, 2018
View reviewed changes
Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

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

looks good 👍

@kbrock we should probably fill in the inverse_of options?

agrare
agrare approved these changes Feb 15, 2018
View reviewed changes
Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

👍 LGTM

I wish we didnt need this and i could just have the lines below as:
has_many :container_nodes, :through => :ext_management_systems
instead.

Yeah this is the other side of pushing the more specific associations down to the specific managers and not cluttering ExtManagementSystem...trade offs on both sides I think

@agrare
Copy link
Member

agrare commented Feb 15, 2018

we should probably fill in the inverse_of options?

This is the same as the existing associations so if we're going to fix it should be in a follow-up PR

@agrare agrare merged commit f5bc79d into ManageIQ:master Feb 15, 2018
@agrare agrare added this to the Sprint 80 Ending Feb 26, 2018 milestone Feb 15, 2018
@agrare agrare self-assigned this Jul 18, 2018
zeari pushed a commit to zeari/manageiq that referenced this pull request Jul 26, 2018
Revert "Merge pull request ManageIQ#16902 from zeari/fix_hourly_event…
d751566 
…_alerts"

This reverts commit f5bc79d, reversing
changes made to c7d99c9.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agrare agrare agrare approved these changes

@Ladas Ladas Ladas approved these changes

@cben cben cben approved these changes

Assignees

@agrare agrare

Labels
bug providers/containers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zeari @miq-bot @agrare @cben @Ladas