Target and target collection abstractions #14249
Conversation
| @@ -316,4 +316,8 @@ def ems_cluster_refresh_target | |||
| def ems_refresh_target | |||
| ext_management_system | |||
| end | |||
|
|
|||
| def manager_refresh_targets | |||
| ext_management_system.class::EventTargetParser.new(self).parse | |||
There was a problem hiding this comment.
That ends up looking really clean :)
app/models/ems_refresh.rb
Outdated
| _log.warn "Unknown target type: [#{c}]." | ||
| next | ||
| end | ||
|
|
||
| h[c] << id | ||
| end | ||
|
|
||
| # Do lookups to get ActiveRecord objects | ||
| # Do lookups to get ActiveRecord objects or initialize ManagerRefresh::Target for ids that are Hash | ||
| targets_by_type.each_with_object([]) do |(c, ids), a| |
There was a problem hiding this comment.
It took me a while to understand that c is a Class
a is an array?
While you are touching this code, maybe change those names to something more explanatory?
There was a problem hiding this comment.
right, I tried to leave minimal impact to the existing code (so I don't break something :-) )
app/models/ems_refresh.rb
Outdated
| @@ -116,20 +116,32 @@ def self.get_ar_objects(target, single_id = nil) | |||
| targets_by_type = target.each_with_object(Hash.new { |h, k| h[k] = [] }) do |(c, id), h| | |||
| # Take care of both String or Class type being passed in | |||
| c = c.to_s.constantize unless c.kind_of?(Class) | |||
| if [VmOrTemplate, Host, ExtManagementSystem].none? { |k| c <= k } | |||
| if [VmOrTemplate, Host, ExtManagementSystem, ManagerRefresh::Target].none? { |k| c <= k } | |||
There was a problem hiding this comment.
Ok, I must admit I'm quite new to this part of our codebase. How about switching in queue_refresh and add something like get_manager_targets. In the end this method here is called get_ar_targets 😄
There was a problem hiding this comment.
yeah, I was thinking about the rename
app/models/manager_refresh/target.rb
Outdated
| } | ||
| end | ||
|
|
||
| alias id serialize |
There was a problem hiding this comment.
are you doing this, so EmsRefresh can call .id on AR Objects and on this?
app/models/manager_refresh/target.rb
Outdated
| # Loads ManagerRefresh::Target ApplicationRecord representation from our DB, this requires that ManagerRefresh::Target | ||
| # has been refreshed, otherwise the AR object can be missing. | ||
| # @return [ApplicationRecord] A ManagerRefresh::Target loaded from the database as AR object | ||
| def load |
There was a problem hiding this comment.
can you point me to the code where this is used?
There was a problem hiding this comment.
not used yet, this will be a post refresh step, when we tie a Target turned to AR to the Actual EmsEvent. I will need to change the modeling, so it allows modeling of EmsEvent has_many targets (polymorphic)
app/models/manager_refresh/target.rb
Outdated
|
|
||
| # Returns a serialized ManagerRefresh::Target object. This can be used to initialize a new object, then the object | ||
| # target acts the same as the object ManagerRefresh::Target.new(target.serialize) | ||
| def serialize |
There was a problem hiding this comment.
I was thinking more in the Rails and Ruby serialize / marshal way to do it.
If you implement a load and dump method, then you can serialize this Object into a AR column.
This is also more in line with https://ruby-doc.org/core-2.3.0/Marshal.html
There was a problem hiding this comment.
right, that would mean changing also the queuing code. This plugs nicely to the existing queuing code, so I would rather leave it like this.
There was a problem hiding this comment.
Why does it plug nicely?
I'm just proposing to use load and dump as serialization methods
There was a problem hiding this comment.
ah, ok, I thought you've meant implementation
so a load would be a class level method that does .new?
| # @param manager_id [Integer] A primary key of the Manager owning the Target | ||
| # @param event_id [Integer] A primary key of the EmsEvent associated with the Target | ||
| # @param options [Hash] A free form options hash | ||
| def add_target!(association:, manager_ref:, manager: nil, manager_id: nil, event_id: nil, options: {}) |
There was a problem hiding this comment.
I would not add a ! here. add_ clearly indicates that you are modifying it. It's more like Array#push
| end | ||
|
|
||
| # @return [String] A String containing an id of each target in the TargetCollection | ||
| def id |
There was a problem hiding this comment.
somewhere in refresher, for logging purposes of what is about to be refreshed
Ladas
force-pushed
the
target_and_target_collection_abstractions
branch
from
1d2a501 to
c141348
Compare
| @@ -2,6 +2,14 @@ class EmsEvent | |||
| module Automate | |||
| extend ActiveSupport::Concern | |||
|
|
|||
| def graph_refresh(sync: false) | |||
There was a problem hiding this comment.
@durandom maybe manager_refresh instead of graph_refresh?
There was a problem hiding this comment.
👍 yeah, graph_refresh is not yet in the code and manager_refresh is. So, good consistency catch. Go for the rename
|
This pull request is not mergeable. Please rebase and repush. |
Add ManageRefresh::Target class. ManageRefresh::Target serves as an input of a targetted refresh, it can point to any remote and local db object.
Add ManagerRefresh::TargetCollection class, that serves for grouping of the Targets of the same Manager or EmsEvent.
A manager_refresh_targets method on EmsEvent that serves as a helper of invoking Porvider specific code that parses ManagerRefresh::Target objects out of the EmsEvent.
EmsEvent graph_refresh method for automate, that scans the EmsEvent and put all found ManagerRefresh::target objects on a queue for refresh.
Add automate helpers for invoking graph_refresh from a state machine
Tweak get_ar_objects method to work with ManagerRefresh::Target. In a case of a Hash id, ManagerRefresh::Target is initialized instead of the AR object.
Simplify get_ar_objects method processing of ManagerRefresh::Target and rename variables to be more readable
Rename get_ar_objects to get_target_objects
Add proper check for a target_class
Add load/dump methods for serialize&deserialize of the target Object
Use load method for deserialize Target
Specs for get_target_objects method with ManagerRefresh::Target
Ladas
force-pushed
the
target_and_target_collection_abstractions
branch
from
cd902fa to
28ba9e2
Compare
Pack serialize&deserialize as a Rails recommended interface
Ladas
force-pushed
the
target_and_target_collection_abstractions
branch
from
94ca4b4 to
a50132f
Compare
:load to :load_from_db, so it doesn't conflict with self.load name points to manager_ref, to give unique identifier
TargetCollection add_target method doesn't need a bang, it's clear enough this method modifies the collection.
Unit tests for the ManagerRefresh::Target
Unit tests for the ManagerRefresh::TargetCollection
Autofix rubocop issues
|
Checked commits Ladas/manageiq@8fc880f~...e5e35b1 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 spec/models/manager_refresh/target_collection_spec.rb
spec/models/manager_refresh/target_spec.rb
|
|
LGTM 👍 |
|
@Ladas 🏆 🎉 this is great |
Bringing new a ManagerRefresh::Target object, that allowed to be queue for the refresh. And adding ManagerRefresh::TargetCollection, to easily process related targets as well as helpers that allow to call the refresh from automate statemachine,