Add additionalVolumes, additionalVolumeMounts #96
Conversation
|
@neilcar; I'll take a look into that |
|
@Vad1mo Hi! Quick ping on this PR for additionalVolumes - I need it for my PostgreSQL certificates. Thanks! |
WalkthroughThe pull request introduces enhanced configuration options for the n8n Helm chart, focusing on volume management and deployment flexibility. Two new configuration parameters, Changes
Sequence DiagramsequenceDiagram
participant User
participant HelmChart
participant Kubernetes
participant N8nContainer
User->>HelmChart: Configure additionalVolumes
User->>HelmChart: Configure additionalVolumeMounts
HelmChart->>Kubernetes: Generate Deployment
Kubernetes->>N8nContainer: Mount Additional Volumes
N8nContainer->>N8nContainer: Use Mounted Volumes
The sequence diagram illustrates how users can configure additional volumes, which are then processed by the Helm chart and mounted into the n8n container during deployment. 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
🧹 Nitpick comments (5)
charts/n8n/templates/deployment.worker.yaml (1)
76-78: Fix indentation in additionalVolumeMounts conditionalThere's an extra space before the
{{- ifconditional. This should be aligned with other volume mount entries.- {{- if .Values.additionalVolumeMounts }} + {{- if .Values.additionalVolumeMounts }}charts/n8n/templates/deployment.yaml (2)
68-68: Remove trailing whitespaceRemove the trailing whitespace on this line.
- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 68-68: trailing spaces
(trailing-spaces)
98-100: Document volume considerations in values.yamlThe implementation is consistent with other deployment templates. However, to help users effectively use these new features:
- Document common use cases (e.g., PostgreSQL certificates)
- Provide examples of volume configurations
- List any restrictions or considerations
Example documentation for values.yaml:
additionalVolumes: [] # - name: postgres-certs # secret: # secretName: postgres-certs additionalVolumeMounts: [] # - name: postgres-certs # mountPath: /etc/postgres/certs # readOnly: trueAlso applies to: 129-132
charts/n8n/values.yaml (1)
339-340: Add newline at end of fileAdd a newline character at the end of the file to comply with YAML best practices.
additionalVolumeMounts: [] +🧰 Tools
🪛 yamllint (1.35.1)
[error] 340-340: no new line character at the end of file
(new-line-at-end-of-file)
README.md (1)
319-322: Consider using a more secure exampleThe current example uses a hostPath volume, which can pose security risks in production environments. Consider showing an example using ConfigMap or PVC instead.
Example using ConfigMap:
- mountPath: /public name: public additionalVolumes: - - hostPath: - path: /mnt/public - name: public + - name: public + configMap: + name: my-config
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(2 hunks)charts/n8n/templates/deployment.webhooks.yaml(2 hunks)charts/n8n/templates/deployment.worker.yaml(2 hunks)charts/n8n/templates/deployment.yaml(3 hunks)charts/n8n/values.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
charts/n8n/values.yaml
[error] 340-340: no new line character at the end of file
(new-line-at-end-of-file)
charts/n8n/templates/deployment.yaml
[error] 68-68: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
charts/n8n/templates/deployment.webhooks.yaml (1)
76-78: Consider adding schema validation for additionalVolumeMounts
While the implementation is functionally correct, consider adding schema validation in the values.yaml to ensure that volume mount paths don't conflict with existing mounts (e.g., /n8n-config, /n8n-secret).
Consider documenting the following restrictions in values.yaml:
- Volume mount names must be unique
- Mount paths should not conflict with n8n's reserved paths
charts/n8n/values.yaml (1)
333-338: LGTM: Well-structured additionalVolumes implementation
The implementation follows Kubernetes best practices for volume configuration and provides clear documentation.
README.md (1)
290-297: LGTM: Clear and comprehensive documentation
The documentation for additionalVolumes and additionalVolumeMounts is well-written and includes:
- Clear descriptions of the functionality
- Practical example demonstrating real-world usage
- Consistent formatting with the rest of the document
Also applies to: 316-322
| {{- if .Values.additionalVolumes }} | ||
| {{ toYaml .Values.additionalVolumes | indent 8}} | ||
| {{- end }} |
There was a problem hiding this comment.
💡 Codebase verification
Volume name conflicts should be documented in values.yaml
The review concern is valid as there are indeed built-in volumes with names "data", "config-volume", and "secret-volume" used across deployment templates. However, since additionalVolumes is defined as an empty array in values.yaml without documentation about reserved names, this could lead to naming conflicts. Instead of adding validation (which would be complex in Helm templates), this should be addressed through documentation.
- Add documentation in values.yaml warning users about reserved volume names:
- data
- config-volume
- secret-volume
🔗 Analysis chain
Ensure volume names are unique across all volume definitions
The implementation correctly adds additional volumes at the pod spec level. However, consider adding validation to prevent volume name conflicts with built-in volumes like "data", "config-volume", and "secret-volume".
Let me check the values.yaml file to understand how additionalVolumes are defined and if there are any existing validations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for volume name definitions across templates
echo "Checking for volume names in templates..."
rg 'name: "?(data|config-volume|secret-volume)"?' charts/n8n/templates/
Length of output: 1518
Script:
#!/bin/bash
# Check values.yaml for additionalVolumes definition and any validation
echo "Checking values.yaml for additionalVolumes..."
cat charts/n8n/values.yaml | grep -A 10 "additionalVolumes:"
Length of output: 285
|
https://docs.n8n.io/hosting/configuration/configuration-examples/custom-certificate-authority/#docker-cli |
Addresses #95
Summary by CodeRabbit
New Features
Documentation
README.mdto include details on new configuration options and community involvement.Bug Fixes