Add extra containers for postiz #7
Conversation
WalkthroughThe changes in this pull request involve updates to the Helm chart for the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
charts/postiz/values.yaml (2)
26-26: Add documentation and examples for the new configuration options.The new fields
service.additionalPortsandextraContainerswould benefit from:
- Documentation comments explaining their purpose and format
- Example configurations, especially for the oauth-proxy use case mentioned in the PR description
Example documentation:
service: additionalPorts: [] # additionalPorts -- Additional ports to expose in the service # @example: # additionalPorts: # - port: 8080 # targetPort: oauth-proxy # protocol: TCP # name: oauth-proxy extraContainers: [] # extraContainers -- Additional containers to run in the pod # @example: # extraContainers: # - name: oauth-proxy # image: quay.io/oauth2-proxy/oauth2-proxy:v7.4.0 # ports: # - containerPort: 8080 # name: oauth-proxyAlso applies to: 48-48
26-26: Consider adding schema validation for the new fields.To prevent configuration errors, consider adding schema validation using a JSON schema in
values.schema.json.Would you like me to help create a schema that validates:
additionalPorts: Array of valid Kubernetes service port objectsextraContainers: Array of valid Kubernetes container specsAlso applies to: 48-48
charts/postiz/templates/postiz-deployment.yaml (1)
Line range hint
71-75: Fix the volumes logic to preserve uploads-volume when using extraVolumes.The current implementation makes
uploads-volumeandextraVolumesmutually exclusive. This could break functionality if the main container still needs the uploads volume while using extra volumes.Replace the either/or logic with an additive approach:
volumes: - {{- if .Values.extraVolumes }} - {{- toYaml .Values.extraVolumes | nindent 8 }} - {{- else }} - name: uploads-volume emptyDir: {} - {{- end }} + {{- with .Values.extraVolumes }} + {{- toYaml . | nindent 8 }} + {{- end }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
charts/postiz/Chart.yaml(1 hunks)charts/postiz/templates/postiz-deployment.yaml(2 hunks)charts/postiz/templates/postiz-service.yaml(1 hunks)charts/postiz/values.yaml(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- charts/postiz/Chart.yaml
| {{- if .Values.service.additionalPorts }} | ||
| {{- toYaml .Values.service.additionalPorts | nindent 4 }} | ||
| {{- end }} |
There was a problem hiding this comment.
💡 Codebase verification
Port name validation is needed for service configuration
The review comment is valid. After examining the codebase:
- The service template defines a default
httpport name additionalPortsis included without any validation- No validation exists in
_helpers.yamlor schema - The default
values.yamlshowsadditionalPortsas an empty array without example structure or documentation
Since Kubernetes requires unique port names within a service, and there's no validation mechanism in place, this could lead to runtime issues if users define additional ports with duplicate names.
- Add validation in
_helpers.yamlto check for unique port names - Document the
additionalPortsstructure invalues.yamlwith an example - Consider adding a values schema file (
values.schema.json) with JSON Schema validation
🔗 Analysis chain
Consider adding validation for port names.
While the implementation is correct, there's no validation to ensure that port names in additionalPorts don't conflict with the default http port name. Duplicate port names could cause issues in service discovery.
Let's check if the chart has any validation for this:
Consider adding a helper template to validate port names or include validation in the schema file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for port name validation in the chart
# Look for any validation templates or helpers
fd -e yaml -e tpl "^_.*" charts/postiz/templates/
# Check if there's a schema file
fd "values.schema.json" charts/postiz/
Length of output: 128
Script:
#!/bin/bash
# Check the content of _helpers.yaml for any port validation
cat charts/postiz/templates/_helpers.yaml
# Check values.schema.json content for port validation
cat charts/postiz/values.schema.json
Length of output: 1918
Script:
#!/bin/bash
# Check the default values file to understand the service port structure
cat charts/postiz/values.yaml
# Check the complete service template to understand port configuration
cat charts/postiz/templates/postiz-service.yaml
Length of output: 2622
|
Hey @kyrbrbik , thanks for the PR, the way you've implemented looks generally OK. I'm confused by this pattern though, why would people want to add "extraContainers" to this Helm chart, and why not use a separate chart for those containers? Is this a common recommended helm chart best practice that is documented somewhere? Thanks. I'm just seeking to understand why this change is useful. Also, sorry for the long delay in getting back to you - I've had to take December off of maintaining open source code as I've been busy with other life stuff - but I'll be more available now :-) |
|
Hi @jamesread, it's a pretty well used pattern that you can add more containers into the application pod that enhance the functionality of it. |
|
Hey @kyrbrbik , thanks for taking the time to reply. I've seen that k8s doc before, and it reminded me that adding containers into the same pod (like sidecar containers) would be necessarily trickier via another helm chart - but would be made easier with the approach in this PR. When I was reviewing it, I thought the extraContainers would be used for deploying containers outside of the same pod - like postgres, or something different - so I hadn't thought of that! I just approved the lint check which ran fine, so I am merging this PR now. Thanks very much for your clean contribution and comments! |
2 participants
Pull Request Template for Helm Chart
Description
This change adds the option to run side containers in the postiz-app pod.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Tested by installing the helm chart with an oauth-proxy additional container and an additional port for the container in the service
Test Configuration: Bare-metal kubernetes cluster
Checklist:
Additional Information
Does this change introduce any new Kubernetes resource types? No
Does this change modify any existing Kubernetes resource types? Yes
Adds optional side container in the posti-app deployment and optional additional port in the posti-app service
Are there any changes to the values.yaml file? Yes
Adds options for additional containers and service ports
Does this change require any specific Kubernetes permissions or RBAC changes? No
Are there any changes to chart dependencies? No
Have you updated the CHANGELOG.md file? No
Theres no changelog file :)
Additional context
Add any other context or screenshots about the pull request here.
Summary by CodeRabbit
New Features
Updates