core icon indicating copy to clipboard operation
core copied to clipboard

Feature/prefix config aws s3 integration

Open fvgarrel opened this issue 7 months ago • 23 comments

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:

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 running python3 -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:

fvgarrel avatar May 10 '25 17:05 fvgarrel

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar May 10 '25 17:05 home-assistant[bot]

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 close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign aws_s3 Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

home-assistant[bot] avatar May 10 '25 17:05 home-assistant[bot]

I'd expect some tests needs to be updated as well.

tomasbedrich avatar May 10 '25 18:05 tomasbedrich

@tomasbedrich I've already fixed the tests but haven't pushed them yet. I'll push that tomorrow.

fvgarrel avatar May 10 '25 19:05 fvgarrel

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 avatar May 11 '25 15:05 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.

fvgarrel avatar May 11 '25 15:05 fvgarrel

I think that that is a bad practice, why not create multiple buckets instead?

../Frenck

frenck avatar May 11 '25 15:05 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.

fvgarrel avatar May 11 '25 16:05 fvgarrel

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 avatar May 11 '25 16:05 frenck

@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.

fvgarrel avatar May 11 '25 16:05 fvgarrel

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.

JulesThuillier avatar May 14 '25 12:05 JulesThuillier

@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 avatar Jun 03 '25 14:06 harrisonhjones

@harrisonhjones I’m done. The PR is ready and I’m just waiting for it to be reviewed.

fvgarrel avatar Jun 03 '25 20:06 fvgarrel

Thanks! I am eager to see it implemented ❤️! Let's see if reviewers have time soon

jpizquierdo avatar Jun 05 '25 20:06 jpizquierdo

@frenck could you take a look to have this feature in the upcoming releases? Thanks!!

jpizquierdo avatar Jun 15 '25 11:06 jpizquierdo

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.

mkverse avatar Jul 26 '25 05:07 mkverse

Any update on this yet? I am waiting on this feature as well.

ericfitz avatar Sep 23 '25 01:09 ericfitz

@fvgarrel

ericfitz avatar Sep 23 '25 01:09 ericfitz

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 avatar Sep 23 '25 17:09 fvgarrel

@fvgarrel thanks!! If you need help with testing or whatever just let us know

jpizquierdo avatar Sep 24 '25 09:09 jpizquierdo

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.

fvgarrel avatar Oct 17 '25 18:10 fvgarrel

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.

emontnemery avatar Oct 23 '25 07:10 emontnemery