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

Add relationship between [physical switch and physical chassis] and event stream #17661

Merged
merged 1 commit into from
Jul 24, 2018

Conversation

felipedf
Copy link
Member

@felipedf felipedf commented Jul 3, 2018

This PR is able to add a relationship between physical_switch - event_stream and physical_chassis - event_stream

Depends on: ManageIQ/manageiq-schema#229

@felipedf felipedf force-pushed the switch_event branch 3 times, most recently from 304590a to 7b963b6 Compare July 5, 2018 20:08
@felipedf felipedf changed the title Add relationship between physical switch and event stream Add relationship between physical switch, physical chassis and event stream Jul 5, 2018
@felipedf felipedf changed the title Add relationship between physical switch, physical chassis and event stream Add relationship between [physical switch and physical chassis] and event stream Jul 5, 2018
@miq-bot
Copy link
Member

miq-bot commented Jul 5, 2018

Checked commit felipedf@4dc96ee with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 👍

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

rest looks good

@@ -27,6 +27,8 @@ class EventStream < ApplicationRecord

belongs_to :middleware_server, :foreign_key => :middleware_server_id
belongs_to :physical_server
belongs_to :physical_chassis, :inverse_of => :event_streams
belongs_to :physical_switch, :inverse_of => :event_streams
Copy link
Member

Choose a reason for hiding this comment

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

I think I'm missing something here.

I didn't find physical_switch_id or physical_chassis_id in event_streams table.

Is it through physical_server?

(Not sure how many fields we can add to event_streams without things going seriously downhill)

Copy link
Member Author

@felipedf felipedf Jul 12, 2018

Choose a reason for hiding this comment

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

My bad here @kbrock, it is depended on another PR, added it to the description

Copy link
Member Author

Choose a reason for hiding this comment

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

(Not sure how many fields we can add to event_streams without things going seriously downhill)

Agree, I guess it was decided to keep things this way while we design a better architecture for events.
@agrare am I right?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the proposed solution was worse than the problem IMO and if this really becomes an issue we can always delete the middleware relationship keys.

@felipedf
Copy link
Member Author

ManageIQ/manageiq-schema#229 Merged

@@ -1,10 +1,14 @@
class PhysicalChassis < ApplicationRecord
include SupportsFeatureMixin
Copy link
Member

Choose a reason for hiding this comment

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

Is this related to adding the relationship to ems_events?

Copy link
Member Author

@felipedf felipedf Jul 24, 2018

Choose a reason for hiding this comment

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

It is, as EventMixin tries to execute some method from SupportsFeatureMixin on its super class, in that context PhysicalChassis

https://github.com/ManageIQ/manageiq/blob/master/app/models/mixins/event_mixin.rb#L6

Copy link
Member

Choose a reason for hiding this comment

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

Ahh okay thanks @felipedf

@@ -1,10 +1,14 @@
class PhysicalSwitch < Switch
include SupportsFeatureMixin
Copy link
Member

Choose a reason for hiding this comment

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

Same

@agrare agrare merged commit 5080b2f into ManageIQ:master Jul 24, 2018
@agrare agrare added this to the Sprint 91 Ending Jul 30, 2018 milestone Jul 24, 2018
@felipedf felipedf deleted the switch_event branch July 24, 2018 18:54
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.

4 participants