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.

Sign up for GitHub

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

Jump to bottom

Warn when we're running fix_auth in dry run mode #17410

Merged
merged 1 commit into from
Jun 5, 2018
Merged

Warn when we're running fix_auth in dry run mode #17410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions tools/fix_auth/auth_model.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ def run(options = {})
puts "fixing #{table_name}.#{available_columns.join(", ")}" unless options[:silent]
processed = 0
errors = 0
would_make_changes = false
contenders.each do |r|
begin
fix_passwords(r, options)
Expand All @@ -96,6 +97,7 @@ def run(options = {})
display_column(r, column, options)
end
end
would_make_changes ||= r.changed?
r.save! if !options[:dry_run] && r.changed?
processed += 1
rescue ArgumentError # undefined class/module
Expand All @@ -111,6 +113,7 @@ def run(options = {})
end
puts "#{options[:dry_run] ? "viewed" : "processed"} #{processed} records" unless options[:silent]
puts "found #{errors} errors" if errors > 0 && !options[:silent]
puts "** This was executed in dry-run, and no actual changes will be made to #{table_name} **" if would_make_changes && options[:dry_run]
end

def clean_up
Expand Down
9 changes: 9 additions & 0 deletions tools/fix_auth/fix_auth.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,15 @@ def generate_password
raise Errno::EEXIST, e.message
end

def print_dry_run_warning
method = caller_locations.first.label
Copy link
Member

Choose a reason for hiding this comment

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

@jrafanie Did you say that you didn't want to pass parameters to caller_locations?

I think the extra parameters let the kernel build a smaller array vs having to build a big array that is basically thrown away.

Looks like stack overflow also has a similar syntax:

caller_locations(1,1).first.label

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'd rather not have ugly ruby internals leak into a place that will rarely if ever cause a performance problem. If they provided a convenience method to retrieve the first caller location, then it would make sense here but since the alternative (1,1 or a range) isn't exactly elegant, I prefer the original code.

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, if it turns out to be a performance problem, we an always change it but I prefer readability here because I can't imagine this is the performance problem that will break us.

# Move this message up to `run` if the other methods add dry-run support
puts "** #{method} is executing in dry-run mode, and no actual changes will be made **" if options[:dry_run]
end

def fix_database_passwords
print_dry_run_warning

begin
# in specs, this is already setup
ActiveRecord::Base.connection_config
Expand All @@ -65,6 +73,7 @@ def fix_database_passwords
end

def fix_database_yml
print_dry_run_warning
FixDatabaseYml.file_name = "#{options[:root]}/config/database.yml"
FixDatabaseYml.run({:hardcode => options[:password]}.merge(run_options))
end
Expand Down