Add configuring bucket-name from args for RO commands #589 #590
Conversation
Upgrade cassandra-driver version (thelastpickle#582)
Bumped cassandra-driver
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
Hi @azarnovdaniil,
I left a couple blocking comments on this PR. The new bucket_name and prefix cli arguments are conflicting with the higher level ones (Click makes that pretty confusing btw...).
I think we should keep the original ones and expose them in the storage class for read access.
Thanks!
| @click.option('--bucket-name', help='Bucket with backup', required=False, default=None) | ||
| @click.option('--prefix', help='Backup prefix', required=False, default=None) |
There was a problem hiding this comment.
Issue: these two new arguments are creating confusion with the ones defined in the global settings (the ones that will be placed before the command name in the command line).
We'd rather need to remove these and expose both the bucket_name and the prefix as is in the storage object.
Then you can use them in your code through the storage object instead of having to pass it around the methods.
There was a problem hiding this comment.
If I understand correctly your suggestion, I should move bucket_name and prefix into the storage config.
Here:
StorageConfig = collections.namedtuple( 'StorageConfig', ['bucket_name', 'key_file', 'prefix', 'fqdn', 'host_file_separator', 'storage_provider', 'base_path', 'max_backup_age', 'max_backup_count', 'api_profile', 'transfer_max_bandwidth', 'concurrent_transfers', 'multi_part_upload_threshold', 'host', 'region', 'port', 'secure', 'aws_cli_path', 'kms_id', 'backup_grace_period_in_days', 'use_sudo_for_restore', 'k8s_mode'] )
If yes, I had another idea in this PR. Changing config everywhere for a single restore is overkill, in the config, we have a configuration for long-term usages(like a regular backup), and changing the config for restore is a potential problem for the backup procedure(you should return config setting back, you should don't have conflicts between operations, etc).
| @@ -427,7 +430,7 @@ def _build_restore_cmd(self): | |||
| command = 'mkdir -p {work}; cd {work} && medusa-wrapper {sudo} medusa {config} ' \ | |||
| '--fqdn=%s -vvv restore-node ' \ | |||
| '{in_place} {keep_auth} %s {verify} --backup-name {backup} --temp-dir {temp_dir} ' \ | |||
| '{use_sstableloader} {keyspaces} {tables}' \ | |||
| '{use_sstableloader} {keyspaces} {tables} {bucket_name} {prefix}' \ | |||
There was a problem hiding this comment.
Issue: following up on my other comment, the --bucket-name and --prefix args need to be placed before the command name (here restore-node).
There was a problem hiding this comment.
'mkdir -p {work}; cd {work} && medusa-wrapper {sudo} medusa {config} ' \ '--fqdn=%s -vvv {bucket_name} {prefix} restore-node ' \ '{in_place} {keep_auth} %s {verify} --backup-name {backup} --temp-dir {temp_dir} ' \ '{use_sstableloader} {keyspaces} {tables}'
Like this?
There was a problem hiding this comment.
No, like this:
mkdir -p {work}; cd {work} && medusa-wrapper {sudo} medusa {config} ' \ '--fqdn=%s -vvv --bucket-name={bucket_name} --prefix={prefix} restore-node ' \ '{in_place} {keep_auth} %s {verify} --backup-name {backup} --temp-dir {temp_dir} ' \ '{use_sstableloader} {keyspaces} {tables}
|
|
Hi @azarnovdaniil ! I've been doing some grooming and stumbled upon this. Is there something we could do to help you move this forward, provided you find this still useful? |
|
|
@rzvoncek Hi! Probably will be easier to reopen this PR than resolve all conflicts with the fresh master branch. |
|
Hi @azarnovdaniil ! I've finally managed to get to this. I took your branch and made one moster of a rebase, and a few small tweaks. I've ended up with #752, which is megeable, but I haven't tested it. I'm also going away till May, and sadly won't get to do more on this. While I'm gone, could you please give my PR a look to see if it works for you? |
Fixes #589