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

Store created updated and deleted records #15603

Merged

Conversation

Ladas
Copy link
Contributor

@Ladas Ladas commented Jul 19, 2017

Store created updated and deleted records. This information can be than used in post refresh to e.g. queue up more jobs for created items. Or by another graph node for post processing to e.g. assign/delete records from folders.

Ladas added 4 commits July 19, 2017 13:23
Allow IC to store what was created/updated/deleted
Unify default and concurrent_safe savers and store counts of
what was created/updated/deleted.
Store counts for concurrent safe batch saver. Counts and ids of
what was created/updated/deleted.
Test that we correctly store what was created/updated/deleted in IC
@Ladas
Copy link
Contributor Author

Ladas commented Jul 19, 2017

@miq-bot assign @agrare
@miq-bot add_label enhancement

cc @cben this can be used for post refresh processing of image events and tagging. (for old refresh, we will probably need to do it a different way)

# Returns a hash with a simple record identity
def record_identity(record)
{
:id => record.try(:[], :id) || record.try(:id)
Copy link
Contributor

Choose a reason for hiding this comment

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

in what scenario could both try fail? if it's possible, we may get one or more {:id => nil} in the arrays, not very useful. actually record.id may also be nil if for some reason it's unsaved.
Can we make it raise exceptions, and elsewhere assume all records in these arrays will have id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, it should not fail. The storing is only on places with actual created/updated/deleted records.

But I can add the exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Ladas added 3 commits July 19, 2017 13:55
Move store_created_records code after records creation
Enforce that identity of the create/updated/deleted record is present
Correct storing of deleted and updated records for a batch strategy

# Returns a hash with a simple record identity
def record_identity(record)
identity = record.try(:[], :id) || record.try(:[], "id") || record.try(:id)
Copy link
Member

Choose a reason for hiding this comment

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

This looks overly complicated, it seems like the caller has a better idea of how to get the id out of what its storing than this. Would it make more sense to pass in the id for the created/updated/deleted records?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, I would rather have it here, since I can later easily add more params, like manager_ref

@miq-bot
Copy link
Member

miq-bot commented Jul 19, 2017

Checked commits Ladas/manageiq@bfcdaca~...42f74aa with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 1 offense detected

app/models/manager_refresh/save_collection/saver/base.rb

@@ -3,55 +3,6 @@ module Saver
class ConcurrentSafe < ManagerRefresh::SaveCollection::Saver::Base
private

def save!(inventory_collection, association)
Copy link
Member

Choose a reason for hiding this comment

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

+1 for consolidating the save! methods now

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 👍

@agrare agrare merged commit 41eb953 into ManageIQ:master Jul 19, 2017
@agrare agrare added this to the Sprint 65 Ending Jul 24, 2017 milestone Jul 19, 2017
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