-
Notifications
You must be signed in to change notification settings - Fork 897
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
For database dumps don't modify the directory name #18058
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a suggestion, the rest of the code seems fine.
lib/evm_database_ops.rb
Outdated
"db_dump" | ||
else | ||
"db_backup" | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is my only suggestion. Just don't like how this looks currently (question if my suggestion is better...), but I am a bit squeamish about passing a []
to File.join
(seems odd):
folder = nil
folder ||= action == :dump ? "db_dump" : "db_backup" unless connect_opts[:skip_directory]
uri = File.join(*[connect_opts[:uri], folder, connect_opts[:remote_file_name].compact)
I would maybe suggest moving this to a method, but since we are doing class methods here (not necessarily a fan of how this class is built) we would have to pass action
and connect_opts
as well, so I did it this way for less complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File.join("a", %w(b c), nil, "d")
TypeError: no implicit conversion of nil into String
from (irb):1:in `join'
from (irb):1
from /opt/rubies/ruby-2.3.7/bin/irb:11:in `<main>'
File.join("a", %w(b c), [], "d")
# => "a/b/c/d"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put in a work around, hope this one looks better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kbrock just an FYI, I was using the *
(splat) operator on a compacted array (removes nil
values), so that is why my sample worked, where your example does not.
But, I think what you have changed it to works for me, so lets go with that. Thanks. 👍
c96a353
to
dbf2ada
Compare
@@ -57,7 +58,7 @@ module EvmDba | |||
db_opts | |||
end | |||
|
|||
CONNECT_OPT_KEYS = [:uri, :uri_username, :uri_password, :aws_region, :remote_file_name].freeze | |||
CONNECT_OPT_KEYS = %i(uri uri_username uri_password aws_region remote_file_name skip_directory).freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NO! Boo! 😡
See ManageIQ/manageiq-providers-amazon#434 (comment)
(pedantic, don't actually change this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. 👍
This pull request is not mergeable. Please rebase and repush. |
Typically "db_dump" or "db_backup" is added the remote path for backup upload. This gives the rake task the --skip-directory option so "db_backup" is not added to the target directory name
daeae55
to
77d1616
Compare
fixed merge conflict |
Checked commits kbrock/manageiq@28a79ac~...77d1616 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
For database dumps don't modify the directory name (cherry picked from commit 256bdd6)
Hammer backport details:
|
overview
Typically
"db_dump"
or"db_backup"
is added the remote path for backup upload.This PR gives the rake task the
--skip-directory
option so"db_backup"
is not added to the target directory namerelated to:
before:
after (using this option):