Skip to content

Conversation

MartinKirchner
Copy link
Contributor

Description of the change

Use a helper template to define the containers for app/cronjob.

Benefits

  • The two containers stay in sync.

Possible drawbacks

None known.

Applicable issues

Additional information

Checklist

@MartinKirchner MartinKirchner force-pushed the feature/containerTemplate branch from 998259a to d5fa9ce Compare March 18, 2025 06:43
@MartinKirchner MartinKirchner marked this pull request as ready for review March 18, 2025 06:47
volumeMounts:
{{- include "nextcloud.volumeMounts" . | trim | nindent 12 }}
{{- $containerName := printf "%s-cron" .Chart.Name }}
{{- include "nextcloud.container" ( dict "containerName" $containerName "securityContext" .Values.cronjob.securityContext "rootContext" $ "context" .Values.cronjob ) | nindent 8 }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we need securityContext, if we has already context ?

Copy link
Collaborator

@wrenix wrenix Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh i see -> do you like to move the values.yaml (and make a breaking change / bump to major)?

remembers me on #379 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wrenix If it's okay for you I'd rather remove the securityContext for the app container (as it is deprecated anyhow).

As for the major version - I hope you remember that my actual goal is to move the cron sidecar to a Kubernetes CronJob.

  • Do you think the Kubernetes Cronjob has a chance to make it's way into this chart?
  • If so, does it make sense to combine these two changes in one new major version?
  • What do you propose on how to proceed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wrenix Any answer to my questions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be unrelated but securityContext is not deprecated right?

One more important thing to know about PodSecurityPolicy: it’s not the same as PodSecurityContext.

A part of the Pod specification, PodSecurityContext (and its per-container counterpart SecurityContext) is the collection of fields that specify many of the security-relevant settings for a Pod. The security context dictates to the kubelet and container runtime how the Pod should actually be run. In contrast, the PodSecurityPolicy only constrains (or defaults) the values that may be set on the security context.

The deprecation of PSP does not affect PodSecurityContext in any way.
https://kubernetes.io/blog/2021/04/06/podsecuritypolicy-deprecation-past-present-and-future/

Copy link
Collaborator

@wrenix wrenix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice

- name: {{ .containerName }}
image: {{ include "nextcloud.image" .rootContext }}
imagePullPolicy: {{ .rootContext.Values.image.pullPolicy }}
{{- if .context.command }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use with instatt of if

@wrenix wrenix changed the title Extract a template for the nextcloud container (app/cronjob) fix: Extract a template for the nextcloud container (app/cronjob) Mar 18, 2025
@wrenix wrenix added the 2. developing Work in progress label Mar 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants