ManageIQ / inventory_refresh

Apache License 2.0
1 stars 23 forks source link

Log destroy result when duplicate record is found #120

Closed jaywcarman closed 5 months ago

jaywcarman commented 9 months ago

When a duplicate record is found, a message is logged that the record has been detected and destroyed before attempting to destroy the record. If the destroy returns False, and the record is not destroyed, nothing is logged. This happens in the case of a read-only record.

An alternate solution could be to call destroy! so that an 'ActiveRecord::RecordNotDestroyed' exception is raised if the record is not destroyed, and read-only records could be handled in a rescue block.

miq-bot commented 9 months ago

Checked commit https://github.com/jaywcarman/inventory_refresh/commit/6cb039eb18726a58a5ef2127bc3ce94edc466eb3 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint 1 file checked, 0 offenses detected Everything looks fine. :star:

jaywcarman commented 9 months ago

Realized I put the comma in the wrong place, so I force pushed the following:

diff --git a/lib/inventory_refresh/save_collection/saver/default.rb b/lib/inventory_refresh/save_collection/saver/default.rb
index 49212e4..1dc220c 100644
--- a/lib/inventory_refresh/save_collection/saver/default.rb
+++ b/lib/inventory_refresh/save_collection/saver/default.rb
@@ -44,7 +44,7 @@ module InventoryRefresh::SaveCollection
           # We have a duplicate in the DB, destroy it. A find_each method does automatically .order(:id => :asc)
           # so we always keep the oldest record in the case of duplicates.
           destroyed = record.destroy ? "and destroyed" : "but could not be destroyed"
-          logger.warn("A duplicate record was detected, #{destroyed} inventory_collection:" \
+          logger.warn("A duplicate record was detected #{destroyed}, inventory_collection:" \
                       "'#{inventory_collection}', record: '#{record}', duplicate_index: '#{index}'")
           return false
         else
miq-bot commented 5 months ago

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).

agrare commented 5 months ago

As a follow-up we might be able to get the reason why a record couldn't be destroyed if we used destroy! and caught the exception