docker-db-auto-backup icon indicating copy to clipboard operation
docker-db-auto-backup copied to clipboard

container_names split does not work for LSIO container image names

Open thomascwells opened this issue 3 years ago • 1 comments

I've been trying to get my home lab configured using Ansible, which I'm closely modeling after your wonderful infrastructure repo. I've been able to teach myself Ansible over the past couple months thanks to you and IronicBadger, and now have LISO SWAG with several other services running on my RaspberryPi. Thanks for all your work!

I've now encountered an issue with db-auto-backup.py which I believe is due to slight differences in the naming of my database images vs yours.

Your Mariadb database image for Nextcloud is mariadb:10.5 while mine is lscr.io/linuxserver/mariadb:latest.

I'm still quite new to Python, but as best I can tell, the the rsplit() call in line 71 is producing ['mariadb'] for your image name, while mine is returning ['lscr.io/linuxserver/mariadb'], which of course can't be found in the get_backup_method() lookup step.

I don't think the fix here is too complicated, and I'm happy to put together a pull request (if you want), but I don't know which solution would be preferable:

  1. use the container.name property instead of container.image.tags. Avoids the need for the rsplit but breaks if containers are not named correctly.
  2. more advanced string-splitting to pull the database type out of the longer image names.
  3. more advanced lookup or string matching within the get_backup_method definition.

thomascwells avatar Sep 06 '22 22:09 thomascwells

I built this with the idea that using non-standard database containers should be possible, and it appears I promptly didn't do anything about it...

I think the solution here is actually already implemented, mostly. BACKUP_MAPPING's keys support glob-style syntax for pattern matching. Using * will match anything on the beginning. The problem with this is we'd need to make sure other containers aren't matched.

Perhaps instead we should more explicitly add support for LSIO containers, by adding extra values into BACKUP_MAPPING for */linuxserver/mariadb? That feels like the solution which is going to break the fewest existing setups.

RealOrangeOne avatar Sep 08 '22 08:09 RealOrangeOne

Thanks for the reply! Sorry for the delay. [insert here standard excuses about day jobs and side projects]

So, I think you're suggesting update BACKUP_MAPPING to look something like this?

BACKUP_MAPPING: Dict[str, BackupCandidate] = {
    "postgres": backup_psql,
    "mysql": backup_mysql,
    "mariadb": backup_mysql, 
    "*/linuxserver/mariadb": backup_mysql
}

Seems like it should work. I'll give that a test on my setup and report back.

thomascwells avatar Sep 20 '22 15:09 thomascwells

any updates on this?

DecWebb avatar Feb 21 '23 20:02 DecWebb

Good question. No, it looks like I haven't implemented this test yet in my setup. It's still on my to-do list.

thomascwells avatar Mar 01 '23 02:03 thomascwells