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

[WIP] Add an option to store record changes #115

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

agrare
Copy link
Member

@agrare agrare commented Nov 21, 2022

Add the option to store changes to records that occurred during refresh.

TODO:

  • specs
  • possible to do with batch saving?
  • add a column mask to only store changes the user cares about

@agrare agrare requested a review from Fryguy as a code owner November 21, 2022 20:38
@miq-bot miq-bot added the wip label Nov 21, 2022
@agrare agrare force-pushed the add_store_record_changes branch 2 times, most recently from ca1ad00 to 5ee3f32 Compare December 20, 2022 18:05
@agrare
Copy link
Member Author

agrare commented Dec 20, 2022

Pass a block to to_h instead of calling map.to_h.

NOTE this doesn't work on ruby 2.5 and was causing CI failures. Separately I'd like to drop 2.5 from the supported ruby versions but don't want to do that all in one PR

Comment on lines +180 to +182
# @param track_record_changes [Boolean/Array] By default false. If false no changes to the InventoryObject will be stored,
# otherwise changes to properties enumerated in the array will be stored for use by other inventory collections
# Example: :track_record_changes => [:name, :raw_power_state]
Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE: I went back and forth a lot on a separate attribute to track the filter plus a boolean to turn on/off, versus combining them all in one attribute. On one hand it is handy to only need one attribute (we have a lot of knobs here already) on the other hand could be confusing to have a boolean/array datatype.

@agrare agrare added the enhancement New feature or request label Dec 21, 2022
@@ -214,6 +215,11 @@ def store_deleted_records(records)
@deleted_records.concat(records_identities(records))
end

def store_record_changes(records)
records = [records] unless records.respond_to?(:map)
@record_changes.merge!(records.map { |r| [record_identity(r)[:id], r.changes.slice(*track_record_changes)] }.to_h)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@record_changes.merge!(records.map { |r| [record_identity(r)[:id], r.changes.slice(*track_record_changes)] }.to_h)
records.each_with_object(@record_changes) { |r, h| h[record_identity(r)[:id]] = , r.changes.slice(*track_record_changes) }

@miq-bot miq-bot added the stale label Jun 26, 2023
@miq-bot
Copy link
Member

miq-bot commented Jun 26, 2023

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@miq-bot miq-bot closed this Oct 2, 2023
@miq-bot
Copy link
Member

miq-bot commented Oct 2, 2023

This pull request has been automatically closed because it has not been updated for at least 3 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@kbrock
Copy link
Member

kbrock commented Oct 12, 2023

@agrare Thought this was almost ready (already ready?). Do we want to reopen?

@agrare
Copy link
Member Author

agrare commented Oct 12, 2023

I ran into issues with concurrent_safe batch and wasn't able to make progress

@agrare agrare reopened this Oct 12, 2023
@miq-bot
Copy link
Member

miq-bot commented Oct 12, 2023

Checked commits agrare/inventory_refresh@d454709~...87ab409 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
8 files checked, 1 offense detected

lib/inventory_refresh/inventory_collection.rb

@miq-bot
Copy link
Member

miq-bot commented Jan 15, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

3 similar comments
@miq-bot
Copy link
Member

miq-bot commented Apr 22, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

@miq-bot
Copy link
Member

miq-bot commented Jul 29, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

@miq-bot
Copy link
Member

miq-bot commented Nov 1, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request stale wip
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants