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

Prefix ems_ref with object type in event hash #123

Merged
merged 1 commit into from
Oct 2, 2017

Conversation

agrare
Copy link
Member

@agrare agrare commented Sep 29, 2017

The event handler expects the ems_ref to be prefixed by the object type,
not using the raw ems_ref. This is causing issues adding a real ems_ref
for an event because the process_container_entities_in_event! method is
clearing the key.

The event handler expects the ems_ref to be prefixed by the object type,
not using the raw ems_ref.  This is causing issues adding a real ems_ref
for an event because the process_container_entities_in_event! method is
clearing the key.
@agrare agrare added the bug label Sep 29, 2017
agrare added a commit to agrare/manageiq that referenced this pull request Sep 29, 2017
Use the class name as a prefix for ems_ref removing a collision with the
real ems_ref column

Depends on ManageIQ/manageiq-providers-kubernetes#123
Fixes ManageIQ#16074
@miq-bot
Copy link
Member

miq-bot commented Sep 29, 2017

Checked commit agrare@f706759 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

agrare added a commit to agrare/manageiq that referenced this pull request Sep 29, 2017
Use the class name as a prefix for ems_ref removing a collision with the
real ems_ref column

Depends on ManageIQ/manageiq-providers-kubernetes#123
Fixes ManageIQ#16074
@@ -4,7 +4,16 @@ module ManageIQ::Providers::Kubernetes::ContainerManager::EventParserMixin
included do
def self.event_to_hash(event, ems_id = nil)
_log.debug("ems_id: [#{ems_id}] event: [#{event.inspect}]")
{
ems_ref_key = case event[:kind]

Choose a reason for hiding this comment

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

Consider moving this above line 30

@moolitayer
Copy link

@agrare The fix makes sense. What I don't get is what Is a real ems_ref?
Is that an ems_ref for the event (not the event target)? Where is that value stored?

Do we have a bug or issue for this or is it for something under development?

@moolitayer moolitayer self-assigned this Oct 1, 2017
@cben
Copy link
Contributor

cben commented Oct 1, 2017

ManageIQ/manageiq#16074

Copy link

@moolitayer moolitayer left a comment

Choose a reason for hiding this comment

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

LGTM

@moolitayer moolitayer closed this Oct 1, 2017
@moolitayer moolitayer reopened this Oct 1, 2017
@cben
Copy link
Contributor

cben commented Oct 1, 2017

IIUC this better be merged simultaneously with ManageIQ/manageiq#16076

@moolitayer moolitayer requested a review from cben October 1, 2017 12:19
@moolitayer moolitayer merged commit 5a20b0d into ManageIQ:master Oct 2, 2017
@moolitayer moolitayer added this to the Sprint 70 Ending Oct 2, 2017 milestone Oct 2, 2017
@agrare agrare deleted the fix_event_ems_ref_issue branch March 23, 2020 17:29
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