Warn when we're running fix_auth in dry run mode

[jrafanie]

https://bugzilla.redhat.com/show_bug.cgi?id=1577303

It's very easy to use -d incorrectly as the database name.
We should remove the short option -d (for dry run) since it
conflicts with pg_dump and other tooling for postgresql but
regardless, we should have obvious feedback that we're in dry run
mode.

Fix database.yml output with dry run

$ tools/fix_auth.rb -h localhost -U root --password smartvm --hardcode bogus vmdb_development -d --verbose -y
** fix_database_yml is executing in dry-run mode, and no actual changes will be made**
fixing /Users/joerafaniello/Code/manageiq/config/database.yml.yaml
  /Users/joerafaniello/Code/manageiq/config/database.yml:
    yaml:
viewed 1 records
** This was executed in dry-run, and no actual changes will be made to /Users/joerafaniello/Code/manageiq/config/database.yml **

Fix database passwords with dry run

$ tools/fix_auth.rb -h localhost -U root --password smartvm --hardcode bogus vmdb_development -d --verbose
** fix_database_passwords is executing in dry-run mode, and no actual changes will be made**
fixing authentications.password, auth_key
viewed 0 records
fixing miq_databases.registration_http_proxy_server, session_secret_token, csrf_secret_token
  1:
    session_secret_token: "v2:{...}" => v2:{...}
    csrf_secret_token: "v2:{...}" => v2:{...}
viewed 1 records
** This was executed in dry-run, and no actual changes will be made to miq_databases **
fixing miq_ae_values.value
viewed 0 records
fixing miq_ae_fields.default_value
viewed 0 records
fixing settings_changes.value
viewed 0 records
fixing miq_requests.options
viewed 0 records
fixing miq_request_tasks.options
viewed 0 records

[Fryguy]

I feel like you should add a banner liek "**This is executing in dry-run mode, and no actual changes will be made**"

The "fixing" vs "would fix" seems a little too subtle and those not reading every single line might just scan right past it (I know I would, as I'd probably just look at the table names), and the "Changes would be made to " gets buried (on first glance of the before/after I didn't even see it).

Alternately, prefix each line with something that stands out like **dry-run - would fix

[bdunne]

👍

[jrafanie]

I'll enhance the banner so it's clearer and LOUDER 👍

[kbrock]

👍

[jrafanie]

Ok, finally updated with @Fryguy's suggestion

[jrafanie]

• Line 54, Col 16 - Performance/Caller - Use caller_locations(1..1).first instead of caller_locations.first.

Settle down @miq-bot, you're talking nonsense.

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[jrafanie]

Rebased after #17413 was merged.

[miq-bot]

Checked commit jrafanie@8935d78 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

tools/fix_auth/fix_auth.rb

• ❗ - Line 54, Col 16 - Performance/Caller - Use caller_locations(1..1).first instead of caller_locations.first.

[kbrock]

I'm good with this.

I did notice that the cop seemed to make sense to me.

[kbrock]

@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

[jrafanie]

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.

[jrafanie]

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.