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

Add tool to remove grouping from report results #17589

Merged
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Wrap in a transaction, rollback on dry-run or error
  • Loading branch information
jrafanie committed Jun 18, 2018
commit 51be17337f3be50af8810a38e3b143186f4940c5
43 changes: 27 additions & 16 deletions tools/remove_grouping_from_report_results.rb
Original file line number Diff line number Diff line change
@@ -18,9 +18,11 @@
# Load rails after trollop options are set. No one wants to wait for -h.
Copy link
Member

Choose a reason for hiding this comment

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

You are a developer after my own heart. 😍

Copy link
Member Author

Choose a reason for hiding this comment

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

I waited twice and got :rage1: so I changed it

require File.expand_path('../config/environment', __dir__)

# Wrap all changes in a transaction and roll them back if dry-run or an error occurs.
ActiveRecord::Base.connection.begin_transaction(:joinable => false)

if opts[:dry_run]
puts "Running in dry-run, changes will be rolled back when complete."
ActiveRecord::Base.connection.begin_transaction(:joinable => false)
puts "Running in dry-run, all changes will be rolled back when complete."

at_exit do
Copy link
Member

Choose a reason for hiding this comment

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

Just because I am not sure: I assume that at_exit will still fire after a exit 1, right?

(Nick could probably just go test this in a ruby script himself...)

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, good point. I'll test that

Copy link
Member

Choose a reason for hiding this comment

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

No need, I already did:

$ ruby -e "at_exit { puts 'foo' }; exit 42"
foo
$ echo $?
42

Copy link
Member Author

Choose a reason for hiding this comment

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

I did several tests of dry run so I'm pretty confident it works. It's a little messy how I'm doing it right now but I can't think of a better way to handle the begin/commit/rollback for dry-run/error/exit

ActiveRecord::Base.connection.rollback_transaction
@@ -30,23 +32,32 @@
start = Time.now.utc
total = 0
fixed = 0

MiqReportResult.find_each(:batch_size => opts[:batch_size]).with_index do |rr, i|
break if opts[:count].positive? && i == opts[:count]
next if rr.report.nil? || rr.report.extras.nil?

if rr.report.extras.key?(:grouping)
rr.report.extras.except!(:grouping)
rr.save!
if rr.reload.report.extras.key?(:grouping)
puts "MiqReportResult: #{rr.id} could NOT be fixed"
begin
break if opts[:count].positive? && i == opts[:count]
next if rr.report.nil? || rr.report.extras.nil?
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, just noticed this, but doesn't this next break the total count accuracy?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, good 👁. I'll fix that.


if rr.report.extras.key?(:grouping)
rr.report.extras.except!(:grouping)
Copy link
Member

Choose a reason for hiding this comment

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

I may have told you to use the non-bang version of this... oops...

Copy link
Member Author

Choose a reason for hiding this comment

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

lol, i confirmed it wasn't working and could then make it work using the bang method

rr.save!
Copy link
Member Author

Choose a reason for hiding this comment

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

@NickLaMuro I am using save! now and rescuing on error.

if rr.reload.report.extras.key?(:grouping)
Copy link
Member

Choose a reason for hiding this comment

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

When would this happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was here before I changed line 43 from .save to .save! and added the begin/rescue around it. It's unlikely to happen now but I left this because it seemed odd to leave out one of the three cases below:

"doesn't need fixing"
"fixed"
"could NOT be fixed"

puts "MiqReportResult: #{rr.id} could NOT be fixed"
else
puts "MiqReportResult: #{rr.id} fixed"
fixed += 1
end
else
puts "MiqReportResult: #{rr.id} fixed"
fixed += 1
puts "MiqReportResult: #{rr.id} doesn't need fixing"
end
else
puts "MiqReportResult: #{rr.id} doesn't need fixing"

total += 1
rescue => err
puts "\nWarning: Rolling back all changes since an error occurred on MiqReportResult with id: #{rr.try(:id)}: #{err.message}"
Copy link
Member

Choose a reason for hiding this comment

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

Pandantic: Should this be warn instead of puts?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I had warn but we don't warn elsewhere. 🤷‍♂️

ActiveRecord::Base.connection.rollback_transaction unless opts[:dry_run]
exit 1
end
total += 1
end

puts "Processed #{total} rows. #{fixed} were fixed. #{Time.now.utc - start} seconds"
ActiveRecord::Base.connection.commit_transaction unless opts[:dry_run]
puts "\nProcessed #{total} rows. #{fixed} were fixed. #{Time.now.utc - start} seconds"