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

[Helm] extend functionality #150

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

vriabyk
Copy link

@vriabyk vriabyk commented Oct 18, 2022

  • add support for already existing secret in cluster with benji configuration
  • add fix for templating issue when volumes are empty list
  • add support for initContainers
  • add Charts.lock which is required for ArgoCD to deploy helm dependencies
  • add templates for ceph configuration options

@vriabyk
Copy link
Author

vriabyk commented Oct 25, 2022

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

@elemental-lf
Copy link
Owner

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

@elemental-lf
Copy link
Owner

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
Copy link

@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
Copy link

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 JohnnyMastricht force-pushed the extend-functionality branch 2 times, most recently from bdfa10a to 6d5070e Compare November 24, 2022 19:13
@JohnnyMastricht
Copy link

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

@JohnnyMastricht
Copy link

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

@elemental-lf
Copy link
Owner

I will take a look in the next few days.

@vriabyk
Copy link
Author

vriabyk commented Apr 11, 2023

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

  • support for already existing secret in cluster with benji configuration
  • ability to set initContainers (for example to initialize database or to pre-create minio bucket) and command for benji maint pod (we wanna start nbd server there using it)
  • templates for ceph configuration options

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

Successfully merging this pull request may close these issues.

None yet

3 participants