elemental-lf / benji

Benji Backup: A block based deduplicating backup software for Ceph RBD images, iSCSI targets, image files and block devices
https://benji-backup.me
Other
136 stars 43 forks source link

[Helm] extend functionality #150

Open vriabyk opened 1 year ago

vriabyk commented 1 year ago
vriabyk commented 1 year ago

@elemental-lf pls review this when you have time :)

elemental-lf commented 1 year ago

@vriabyk thanks for your contribution! I will look into it in the next few days.

elemental-lf commented 1 year ago

add support for already existing secret in cluster with benji configuration

I like the idea but I'm not onboard with the same variable value having two different types (boolean and string).

add fix for templating issue when volumes are empty list

I committed a similar fix also taking into account the new Argo Workflow based cronjobs.

add support for initContainers

Could you describe your use case for these? I want to understand which pods need init containers and which don't.

add Charts.lock which is required for ArgoCD to deploy helm dependencies

I'm still unsure about this. I'm using ArgoCD myself and I don't see this behavior. But I'm not using helm directly but via helmfile so that might be the difference. But I also couldn't find any hints in the ArgoCD documentation that this is required. Could you point me to some info about this?

JohnnyMastricht commented 1 year ago

@elemental-lf Hey, thanks for your comments! Let me try addressing some of them:

I like the idea but I'm not onboard with the same variable value having two different types (boolean and string).

It was not intended to use it as boolean, it's just a matter of the external secret object name pasted there. The value could be just empty and meant to save some space. Otherwise it could be something like:

benji:
  externalSecretConfig: true
  secretName: secret

I think it just adds more lines and doesn't necessary makes it cleaner.

I committed a similar fix also taking into account the new Argo Workflow based cronjobs.

Thanks, I'll check that and rebase our code

add support for initContainers

We use some init steps to check or initialize PostgreSQL database that we'll be using later on in our set up. Generally, having initContainers just adds some functionality to the chart that can be used or not used by clients.

As for Charts.lock thing, I'll reply in the next comment

JohnnyMastricht commented 1 year ago

So as for Charts.lock there is this issue. What I think is that for now actually we could ommit this since we also switched to helmfile and we got to test how it works now. We'll add some commits to this PR soon.

JohnnyMastricht commented 1 year ago

@elemental-lf hey, we've changed some things here and there, so please check once you have free time

JohnnyMastricht commented 1 year ago

@elemental-lf hello, is there any news regarding this?

elemental-lf commented 1 year ago

I will take a look in the next few days.

vriabyk commented 1 year ago

to summarize what we are adding here after the whole discussion: