Feature/prefix config aws s3 integration
Breaking change
Proposed change
Added the prefix configuration to aws s3 integration.
Type of change
- [ ] Dependency upgrade
- [ ] Bugfix (non-breaking change which fixes an issue)
- [ ] New integration (thank you!)
- [x] New feature (which adds functionality to an existing integration)
- [ ] Deprecation (breaking change to happen in the future)
- [ ] Breaking change (fix/feature causing existing functionality to break)
- [ ] Code quality improvements to existing code or addition of tests
Additional information
- This PR fixes or closes issue: fixes #144565
- This PR is related to issue: #144565
- Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/38971
- Link to developer documentation pull request:
- Link to frontend pull request:
Checklist
- [x] The code change is tested and works locally.
- [x] Local tests pass. Your PR cannot be merged unless tests pass
- [x] There is no commented out code in this PR.
- [x] I have followed the development checklist
- [x] I have followed the perfect PR recommendations
- [x] The code has been formatted using Ruff (
ruff format homeassistant tests) - [x] Tests have been added to verify that the new code works.
If user exposed functionality or configuration variables are added/changed:
- [x] Documentation added/updated for www.home-assistant.io
If the code communicates with devices, web services, or third-party tools:
- [x] The manifest file has all fields filled out correctly.
Updated and included derived files by running:python3 -m script.hassfest. - [x] New or updated dependencies have been added to
requirements_all.txt.
Updated by runningpython3 -m script.gen_requirements_all. - [x] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
To help with the load of incoming pull requests:
- [ ] I have reviewed two other open pull requests in this repository.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:
Hey there @tomasbedrich, mind taking a look at this pull request as it has been labeled with an integration (aws_s3) you are listed as a code owner for? Thanks!
Code owner commands
Code owners of aws_s3 can trigger bot actions by commenting:
@home-assistant closeCloses the pull request.@home-assistant rename Awesome new titleRenames the pull request.@home-assistant reopenReopen the pull request.@home-assistant unassign aws_s3Removes the current integration label and assignees on the pull request, add the integration domain after the command.@home-assistant add-label needs-more-informationAdd a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.@home-assistant remove-label needs-more-informationRemove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.
I'd expect some tests needs to be updated as well.
@tomasbedrich I've already fixed the tests but haven't pushed them yet. I'll push that tomorrow.
Honestly, I think this is not a good approach or an approach we should advise/hint for. If one has different purposes for storage that shouldn't conflict or be separated, for whatever reason, a different/new bucket should be created instead.
../Frenck
@frenck I don’t quite understand what you’re trying to say with your comment. So you’re against using prefixes? Could you explain why you think it is is a bad approach?
Some people use a single bucket for all kinds of backups. I’ve organized mine using prefixes. I would prefer not to create a separate bucket just for Home Assistant.
I think that that is a bad practice, why not create multiple buckets instead?
../Frenck
@frenck There’s no specific reason why I only use one bucket.. it’s more a subjective personal preference to handle it that way.
I can understand that it’s considered as bad practice, but I think some people would be happy about it.
Because the general idea behind buckets, it that they can be created cheaply (as in, low effort), to hold a specific domain of files / bundle areas of responsibility. Thus this prefix seems to workaround this concept?
@frenck I think it depends on how you define a domain for yourself. For me, ‘backups’ is its own domain. Since Amazon handles prefixes and lets you build policies around them, I think they have their place to be in certain situations.
This would be quite useful indeed : we use many home assistant installation and to avoid having to setup each device from scratch we restore a golden backup. But at the moment we have to create a new bucket, change the config on each device which can be time consuming and prone to human errors. At least including a device mac or device name in the backup would be very useful.
@fvgarrel where are you on this? I'm eagerly awaiting this feature as someone who manages multiple instances. Anything I can do to help?
@harrisonhjones I’m done. The PR is ready and I’m just waiting for it to be reviewed.
Thanks! I am eager to see it implemented ❤️! Let's see if reviewers have time soon
@frenck could you take a look to have this feature in the upcoming releases? Thanks!!
Any update on this?
@fvgarrel I see that the review process of HomeAssistant didn't accept this because of the email associated to the commits not being linked to a GitHub account
I think the easiest solution is to just do that and link that email to your GitHub account. Not sure how to retrigger the review should you have already done this.
Any update on this yet? I am waiting on this feature as well.
@fvgarrel
Hey, I’ll have a look in a few days. I’m quite busy at the moment. But I want to finish this PR! 🙂
@fvgarrel thanks!! If you need help with testing or whatever just let us know
Please add tests of the migration. Also, I think tests need to be updated to show the prefix is used as intended,
@emontnemery I’m not sure which tests I should add. Could you clarify what kind of tests you had in mind? The prefix is just a "extension" of the key for the file. I cant imagine how you could use it wrong.
I’m not sure which tests I should add. Could you clarify what kind of tests you had in mind? The prefix is just a "extension" of the key for the file. I cant imagine how you could use it wrong.
@fvgarrel your PR is adding code to allow the user to pick a prefix, and to use the prefix in calls to the aws_s3 client. However, you don't update tests to make sure that:
- The picked prefix is stored in config entry data
- The stored prefix is used in API calls
- Old config entries without a stored prefix still work
If it's not clear to you how to update the tests, let me know and I'll give some suggestions. If you have Discord, please PM me there, it'll probably be easier to solve this in chat.
I'm setting the PR to draft for now.