-
Notifications
You must be signed in to change notification settings - Fork 897
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 a 'Container Project Discovered' event #16903
Conversation
@miq-bot add_label wip |
@@ -35,6 +35,8 @@ class ContainerProject < ApplicationRecord | |||
virtual_total :containers_count, :containers | |||
virtual_total :images_count, :container_images | |||
|
|||
after_create :raise_creation_event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In graph refresh batch saving, we do not call these, you can add it in:
https://github.com/Ladas/manageiq-providers-kubernetes/blob/fb3e5d8b1525d54319dcefecd914e1b135f75a1e/app/models/manageiq/providers/kubernetes/container_manager/refresher_mixin.rb#L86
so in similar way as for container_images, we can access persistor.container_projects.created_records.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7a7f2ec
to
40396c5
Compare
@miq-bot remove_label wip |
I tested this out and it looks fine (with the other PRs from other repos). |
@@ -26,6 +26,7 @@ class MiqAlert < ApplicationRecord | |||
ExtManagementSystem | |||
MiqServer | |||
ContainerNode | |||
ContainerProject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add a miq_alert_statuses relation to container_project.rb
has_many :miq_alert_statuses, :as => :resource, :dependent => :destroy
Generally looks good, It would be helpful if you could separate the pieces related to the new event from the policy/alert bits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the relation but I didnt split the PR since this is a small feature that already has 3 PRs associated with it.
I think its a little too much noise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change caused ui-classic travis failures, fixed in ManageIQ/manageiq-ui-classic#3382
ccc4935
to
d7deca9
Compare
Checked commits zeari/manageiq@40396c5~...d7deca9 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0 |
LGTM 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice @zeari LGTM
(Should this become |
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1469138
Completed by:
ManageIQ/manageiq-providers-kubernetes#226
ManageIQ/manageiq-ui-classic#3360
This is the exact same implementation as 'Container Image Discovered'
If we like this type
Discovered
events then ill refactor this and the code forContainerImage
into aDiscoveredEventMixin
.@cben @moolitayer @Ladas PTAL
cc @oourfali
@miq-bot add_label providers/containers, bug