add security context config to mariadb-isalive and postgresql-isready…

[raynay-r]

Pull Request

Description of the change

The SecurityContext configuration was not applied to the InitContainers before. This PR fixes this.

Benefits

The SecurityContext is applied to all containers in the main Deployment and can therefore be started in Openshift with the correct configuration.

Possible drawbacks

SecurityContext is the same for all containers of the main Deployment and can not be configured individually. Although, this is not a issue introduced here and should not be a problem.

Checklist

• [x] DCO has been signed off on the commit.
• [x] Chart version bumped in Chart.yaml according to semver.
• [ ] (optional) Variables are documented in the README.md

[jessebot]

Sorry for the delay! Looks like there's a conflict to resolve, but this otherwise seems like a fine change. The only thing I could think to imrpove it would be to explicitly add a parameter for this, but not sure if that is excessive to add a securityContext to an init container 🤔

[raynay-r]

@jessebot Sorry, I completely forgot about this PR. I also think that it is not necessary to add extra parameters for the init container, but I could create them if you like. The conflict is only with the version number. I think that's best fixed before merging, otherwise the conflict will appear again when another branch is merged earlier.

[jessebot]

@jessebot Sorry, I completely forgot about this PR.

Me too 🤦 No problem at all!

The conflict is only with the version number. I think that's best fixed before merging, otherwise the conflict will appear again when another branch is merged earlier.

That's fair! There was actually a massive linting PR that got merged recently so there's also a deployment.yaml conflict now, but it should just be indentation and the use of with instead of if in templating.

I also think that it is not necessary to add extra parameters for the init container, but I could create them if you like.

Yeah, lets create those parameters in the values.yaml, just in case, because I would rather it be configurable, just in case. Sorry to add more work. I always feel bad when I come back ages later and request more stuff, but I can only dedicate lots of hours to this repo every couple of months 🤦

[raynay-r]

I finally got around to adding the extra parameters.

Would be great if you could take a look and sorry for the long delay (again).

[jessebot]

@raynay-r eventually we will get this merged! Please update the version in Chart.yaml for a minor version and then we can probably merge this :)

[raynay-r]

@jessebot the version update is done

[jessebot]

Merged in upstream changes and suggestions, but there's a commit here that needs a sign off before we can merge this in.

[raynay-r]

Wow. I tried to sign the commits by rebasing and fucked up royally :D

Will try to fix it in the next few days...

[raynay-r]

Ok, I recoverd the old state. The only way I know to sign these commits would be an interactive rebase, but that makes problems since there are merge commits in between.

The only solution that comes to mind is to squash all commits and sign the resulting one.

Do you have a better idea?

[provokateurin]

Squashing everything SGTM

[raynay-r]

Squashed and signed. I also incremented the chart version to the next minor version.

[provokateurin]

I just thought about this again and I think it might make sense to convert these two key-value pairs into proper maps, such that they can be extended later if there are more options to set for the init-containers:

nextcloud:
  mariaDbInitContainer:
    securityContext: {}
    ... Possible future settings

@raynay-r could you adapt the PR to include these changes? This makes future maintenance and additional features a bit easier.

[provokateurin]

You need to fix the DCO and while you're at it you can squash everything again.

[raynay-r]

I just thought about this again and I think it might make sense to convert these two key-value pairs into proper maps, such that they can be extended later if there are more options to set for the init-containers:

nextcloud:
  mariaDbInitContainer:
    securityContext: {}
    ... Possible future settings

@raynay-r could you adapt the PR to include these changes? This makes future maintenance and additional features a bit easier.

I applied you recommendation. The only issue I see with it in the moment is that if for example mariaDbInitContainer is null or the securityContext value is not available, the templating step will crash. Since all of that is set in the values.yaml it should be fine, but if you want additional null checks, let me know.

[provokateurin]

I think that is fine, if you don't break it it's not broken :woman_shrugging: