Generalize s3-backup #5
Conversation
|
Thanks for the pull request. Very much appreciated!! I will take a look into it this weekend |
|
@thomasfr i hope it's okay, that i've added also an option to set the max_concurrent_requests param for aws. |
|
@thomasfr do you had time for a look? :) |
There was a problem hiding this comment.
Hey, sorry for my late reply. Had a car accident recently and was off.
Nevertheless, thank you very much for your changes.
In general, i understand why you want to rename it. I do not want to rename though, let me explain why:
By default it uses Amazon S3, the Amazon S3 APIs, the official aws cli and will work that way by default. Additionally, i personally think its easier to search for and easier to understand what it does - since "S3" is not explicit enough imho.
So please remove all changes where you remove Amazon and AWS.
Besides that, i would like to keep also the cli command as explicit as possible with all used arguments and parameters. In this scenario, i do not see any benefits of setting the parameters first in the awscli configuration.
Thanks again, looking forward for your changes. Will then merge as asap as possible ;)
s3-backup/config.json
Outdated
| @@ -20,6 +20,8 @@ | |||
| "options": { | |||
| "aws_access_key": "", | |||
| "aws_secret_access_key": "", | |||
| "endpoint_url": "", | |||
| "max_concurrent_requests": 10, | |||
There was a problem hiding this comment.
The default value is already 10 (https://docs.aws.amazon.com/cli/latest/topic/s3-config.html#max-concurrent-requests).
Could you make it optional, the same way you are doing it with endpoint_url? This way, one can just leave it as is and be sure that the default value is used by the cli by default.
s3-backup/run.sh
Outdated
| export AWS_SECRET_ACCESS_KEY="$(bashio::config 'aws_secret_access_key')" | ||
| export AWS_REGION="$bucket_region" | ||
| bashio::log.info "Configure S3 Backup..." | ||
| aws configure set aws_access_key_id "$(bashio::config 'aws_access_key')" |
There was a problem hiding this comment.
I do not think this is necessary. I am not aware that there is a benefit of setting it, especially in this situation since its a one-off script anyways. Could you revert this change please?
s3-backup/run.sh
Outdated
| aws configure set s3.max_concurrent_requests "$(bashio::config 'max_concurrent_requests')" | ||
| fi | ||
|
|
||
| aws configure set region "$(bashio::config 'bucket_region' 'eu-central-1')" |
There was a problem hiding this comment.
Please do not set region like that, i would like to be as explicit as possible with the cli command as it makes it easier to reason on one line what will happen. Otherwise a reader of the code has to go through the whole command or log outputs.
s3-backup/run.sh
Outdated
| endpoint_url="$(bashio::config 'endpoint_url' '')" | ||
| bashio::log.info "Configure S3 Backup endpoint to ${endpoint_url}" | ||
| aws configure set s3.endpoint_url "${endpoint_url}" | ||
| aws configure set s3api.endpoint_url "${endpoint_url}" |
There was a problem hiding this comment.
I would highly prefer having this set as cli parameters. This way we can output pretty much all used parameters on line (https://github.com/thomasfr/hass-addons/pull/5/files#diff-06c5463fab0981ee606b95f4aee978538574d4d7a9884998c868a4f6370273a6R34)
|
Really looking forward to seeing this merged. Thanks for your work! :) |
|
sorry for my long delay, i was a little bit busy the last weeks 😒 |
|
@thomasfr i reverted the renaming stuff. This is the reason why i think, it's better to use the |
|
Hi there! |
|
Hi, |
|
Keen to use cloudflare R2 with this PR 😄 |
Closes #2 😁☺️
I run my changes with scaleways object storage