Added sidecar #386

2022-12-28T08:39:27Z

JSchlarb commented

2022-12-28 08:39:27 +00:00

(Migrated from gitea.com)

Description of the change

Be able to add additional containers (sidecars) to gitea pod. In my case I will use it for backup purpose as I can't create backups from its volume directly.

Benefits

Allow users to add additional containers on their behalf

Possible drawbacks

None. It won't render until .Values.sidecars has a values that renders more than a newline

Applicable issues

None. It won't render until .Values.sidecars has a values that renders more than a newline.

Checklist

Parameters are documented in the values.yaml and added to the README.md using readme-generator-for-helm
Breaking changes are documented in the README.md

### Description of the change Be able to add additional containers (sidecars) to gitea pod. In my case I will use it for backup purpose as I can't create backups from its volume directly. ### Benefits Allow users to add additional containers on their behalf ### Possible drawbacks None. It won't render until `.Values.sidecars` has a values that renders more than a newline ### Applicable issues None. It won't render until `.Values.sidecars` has a values that renders more than a newline. ### Checklist  - [x] Parameters are documented in the `values.yaml` and added to the `README.md` using [readme-generator-for-helm](https://github.com/bitnami-labs/readme-generator-for-helm) - [x] Breaking changes are documented in the `README.md`

pat-s (Migrated from gitea.com) reviewed 2022-12-28 10:35:15 +00:00

pat-s (Migrated from gitea.com) left a comment

Thanks!

I don't mind this change. Some comments:

I would add it as a top-level option as it is not related to the gitea config itself
Name it sidecar instead of sidecars - it is uncommon to use the plural form even if you can use list notation for a config item
I think we should add more description both in values.yml and maybe in the README to describe it's purpose and maybe add a short example. If you already have one yourself, maybe that's not much of a big deal for you? I.e. just showcasing your config as an example.

Thanks! I don't mind this change. Some comments: - I would add it as a top-level option as it is not related to the gitea config itself - Name it `sidecar` instead of `sidecars` - it is uncommon to use the plural form even if you can use list notation for a config item - I think we should add more description both in `values.yml` and maybe in the README to describe it's purpose and maybe add a short example. If you already have one yourself, maybe that's not much of a big deal for you? I.e. just showcasing your config as an example.

justusbunsi commented

2023-01-14 13:06:35 +00:00

(Migrated from gitea.com)

All containers should take the security context into account.

jouve commented

2023-03-01 13:22:58 +00:00

(Migrated from gitea.com)

@pat-s the chart is already using mostly plurals (extraVolumes, extraContainerVolumeMounts, ...).

this is also in accordance with k8s resources (example, a pod containers, initContainers, volumes, etc)

ref: https://gitea.com/gitea/helm-chart/src/branch/main/values.yaml#L224

@pat-s the chart is already using mostly plurals (extraVolumes, extraContainerVolumeMounts, ...). this is also in accordance with k8s resources (example, a pod containers, initContainers, volumes, etc) ref: https://gitea.com/gitea/helm-chart/src/branch/main/values.yaml#L224

pat-s commented

2023-03-01 14:03:45 +00:00

(Migrated from gitea.com)

@jouve OK I see, I've also found other charts that use the plural here. In general it looks like that it's more a wild mix than a clear consistency overall wether to use singular or plural.

justusbunsi commented

2023-03-01 18:47:31 +00:00

(Migrated from gitea.com)

There are several use cases. Imagine a container taking care of changes in a configuration file and sends requests via Gitea API to configure Gitea. Or another container that is tightly coupled with Gitea and receives a webhook to do something (not CI) and it is not necessary to have that endpoint accessible outside of Gitea.

pat-s commented

2023-03-02 12:22:45 +00:00

(Migrated from gitea.com)

Oh I was not questioning the use cases, just discussing singular/plural notation.

We still miss the sec context point you raised, Justus. Not sure though if OP is still active here.

Oh I was not questioning the use cases, just discussing singular/plural notation. We still miss the sec context point you raised, Justus. Not sure though if OP is still active here.

justusbunsi commented

2023-03-07 15:52:52 +00:00

(Migrated from gitea.com)

@JSchlarb do you still want to continue this Pull Request? There are some unresolved topics and a conflict that should be resolved.

pat-s commented

2023-05-03 07:44:18 +00:00

(Migrated from gitea.com)

I am closing here for now as the PR seems stale, please comment if it should be reopened.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: lunny/helm-chart#386