Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify the whole code #3

Open
moufmouf opened this issue Sep 12, 2019 · 0 comments
Open

Simplify the whole code #3

moufmouf opened this issue Sep 12, 2019 · 0 comments

Comments

@moufmouf
Copy link

Hey @BertrandGouny !

I'm using your image on a regular basis and today, and first, I would like to thank you for your work. I encountered an issue today. I wanted to be able to configure MYSQL backups and S3 uploads.
Those features are available in "backup-manager" but you don't expose the environment variables "BACKUP_MANAGER_XXX" in your Docker image.

I started writing a PR to add support for those variables. I started editing the startup.sh file to add all environment variables.

https://github.com/osixia/docker-backup-manager/blob/stable/image/service/backup-manager/startup.sh#L17-L27

And then it hit me.
I've had a look at the code of backup-manager itself.
It is actually a simple collection of shell scripts.

And look: https://github.com/dkogan/Backup-Manager/blob/master/backup-manager#L200
The configuration file as actually a shell script, and all lines in the configuration file are actually environment variables.

So instead of trying to build a configuration file at startup, it would be way easier to simply delete the configuration file and document that you can use any configuration options of backup-manager as an environment variable passed in Docker.

Actually, this is not 100% true, you will face issues with the environment variables types as arrays (that are not supported by Docker). But you could definitely shrinken your configuration file by only keeping these lines: https://github.com/osixia/docker-backup-manager/blob/stable/image/service/backup-manager/assets/backup-manager.conf#L281-L316

Everything else can be removed.

Benefits:

  • less code
  • so less bugs
  • easier migration to new versions of backup-manager
  • more features!

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant