Add pg_dump support back to PostgresAdmin

[NickLaMuro]

Adds PostgresAdmin.backup_pg_dump, which mostly undoes the changes in 271febd3 (from #302 ).

In addition:

• Refactors out PostgresAdmin.combine_command_args
• Uses that method in PostgresAdmin.runcmd
• Updates PostgresAdmin.backup_pg_dump to use .combine_command_args and a new method .handle_multi_value_pg_dump_args! to handle arguments for pg_dump that can have multiple values, and passes them to AwesomeSpawn in a manner that pg_dump should be happy with.

The end result is that .backup_pg_dump will call to runcmd_with_logging directly instead of runcmd, but that is just a implementation detail.

Remaining questions

• Do we want to make PostgresAdmin.backup default to PostgresAdmin.backup_pg_compress by default, but if it gets any -t or --table type args, use pg_dump instead? (I am indifferent, but we are kind of doing this already with restore, so I figured I would ask)
• Related to the above: Since this is for BZ#1535345, do we want to add another option to the appliance_console to differentiate between Database Backup and Database Dump?
  • A: We will be differentiating between "dump"/"backup", as far as I can tell. I will make sure to add a "WARNING" @carbonin 's suggestion here

Links

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

[carbonin]

pg_dump should not be an option for backup.

The resolution for https://bugzilla.redhat.com/show_bug.cgi?id=1535345 should utilize pg_basebackup

Ah this is for support, not for backup. We should just ensure that this is clear, maybe add a warning in the console.

[miq-bot]

Checked commits NickLaMuro/manageiq-gems-pending@9dcbe55~...156e1ec 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

spec/util/postgres_admin_spec.rb

• ❗ - Line 116, Col 50 - Style/PercentLiteralDelimiters - %w-literals should be delimited by ( and ).

[NickLaMuro]

@carbonin Seeing as the requirement is now clearly defined for https://bugzilla.redhat.com/show_bug.cgi?id=1535345#c20 , and the request is to use a pg_dump, is this okay now? Is there anything else you would like to see changed here?

[carbonin]

So as to your first question:

Do we want to make PostgresAdmin.backup default to PostgresAdmin.backup_pg_compress by default, but if it gets any -t or --table type args, use pg_dump instead?

I think that we should keep them as separate methods, especially given the confusion I want to avoid around backup vs. dump.

[carbonin]

@NickLaMuro are we shooting for table exclusions on the first pass? The comment in the BZ made that seem more like a "nice to have".

I'm okay leaving it in here either way. I think the challenging part will be making the console UX make sense so we can probably make that decision in that PR. What do you think?

[NickLaMuro]

I think that we should keep them as separate methods, especially given the confusion I want to avoid around backup vs. dump.

Cool, works for me.

@NickLaMuro are we shooting for table exclusions on the first pass?

Well, first pass as in "the resulting work for this sprint", then yes, only because I don't see it as being much more work. I am probably going to split it up PR wise though, so it will probably look something like this for you:

• Merge this PR to get pg_dump functionality in PostgresAdmin
• PR on manageiq to create evm:db:dump:local and evm:db:dump:remote tasks (mimicking the restore and backup commands that already exist. Should be a quick one.
• PR to take the work I already did for backup and put that into the appliance_console (just was about to open this PR). Not technically part of the BZ, but seems like it doesn't hurt to have it. (PR link)
• PR on appliance_console to add dump functionality that simply does the same steps as restore and backup. Should be straight forward and built on top of the backup one.
• Follow up PR(s) to appliance_console to add things like "table blacklisting" and "file splitting" to the dump action specifically.

I also see (down the road) making some PRs to add FTP support to these actions and the rake tasks, but that is part of the additional RFE as talked about in the BZ.

I think the challenging part will be making the console UX make sense so we can probably make that decision in that PR.

Agreed, though I think we have some facilities already in the Prompts module that should help with this (repeating questions and what not), so I don't think it will be too terribly difficult. But I will throw out a PR that has kind of my first idea for this and we can refine that from there, when I get to that point of course.

[carbonin]

Sounds good to me @NickLaMuro