feat(helm): support external MariaDB#1068
feat(helm): support external MariaDB#1068thibaultamartin wants to merge 10 commits intogrimmory-tools:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds conditional enabling for the MariaDB subchart, introduces a commented externalDatabase values block, adds Helm template validation and helper templates to resolve DB connection parameters from either the bundled MariaDB or an external database, and wires initContainer and app envs to the resolved DB and secret. ChangesExternal Database Support
Sequence Diagram(s)sequenceDiagram
participant Helm as Helm renderer
participant Template as Deployment template
participant Init as initContainer (wait-for-database)
participant App as App container
participant DB as Database (MariaDB or External)
Helm->>Template: render with values (mariadb.enabled, externalDatabase.*)
Template->>Template: validate config (mutual exclusion / presence)
Template->>Template: compute dbHost, dbPort, dbName, dbUserName, dbPasswordSecretName, dbPasswordSecretKey
Init->>DB: probe dbHost:dbPort until ready
App->>DB: connect using computed URL and secretKeyRef for password
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…cy or externalDatabase
|
This PR has been successfully tested against a cluster with a pre-existing MariaDB database deployed via the mariadb-operator. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/helm/grimmory/templates/deployment.yaml (1)
73-107: 🧹 Nitpick | 🔵 TrivialOperational note — update linked documentation for the
externalDatabasepath.The linked repository (
grimmory-tools/grimmory-docs) currently assumes a bundled MariaDB service reachable at hostmariadbwith a hardcodedDATABASE_URL. This PR changes the bundled hostname to{release-name}-mariadband introduces an entirely newexternalDatabaseconfiguration surface. Theinstallation.md,getting-started.md, andtesting-setup.mdfiles ingrimmory-docsshould be updated to document: the newexternalDatabase.*values, the requirement for a pre-existing Kubernetes Secret when usingexternalDatabase, and the updated defaultDATABASE_URLhostname for the bundled case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/helm/grimmory/templates/deployment.yaml` around lines 73 - 107, Update the documentation to reflect the deployment template changes: document the new externalDatabase.* configuration fields (externalDatabase.host/port/name/username/passwordSecretName/passwordSecretKey), explain that the bundled DATABASE_URL default host now uses the release-scoped host pattern (the template's $dbHost value, e.g. {release-name}-mariadb) and the default port/name matching $dbPort/$dbName, and add a note that switching to externalDatabase requires a pre-created Kubernetes Secret referenced by the template's dbPasswordSecretName/dbPasswordSecretKey (used to populate DATABASE_PASSWORD); also update installation.md, getting-started.md, and testing-setup.md to show examples for both bundled and externalDatabase setups and the corresponding environment variables DATABASE_URL, DATABASE_USERNAME and DATABASE_PASSWORD.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/helm/grimmory/templates/deployment.yaml`:
- Around line 29-31: The template assigns $dbPasswordSecretKey from
.Values.mariadb.auth.secretKeys.userPasswordKey without validation, which can
yield an empty secretKeyRef.key; change the assignment to use the Helm required
function (e.g. set $dbPasswordSecretKey = required "message"
.Values.mariadb.auth.secretKeys.userPasswordKey) so that when
.Values.mariadb.auth.existingSecret is set but the user omitted
secretKeys.userPasswordKey Helm will fail with a clear error; update the message
to mention mariadb.auth.existingSecret and secretKeys.userPasswordKey and
reference $dbPasswordSecretKey and the resulting secretKeyRef.key in the
message.
- Around line 100-107: The YAML interpolations for the DB env vars are not
quoted and can be broken by YAML metacharacters; update the deployment template
so each user-controlled template expression uses the Helm quote pipe (| quote) —
specifically wrap {{ $dbHost }}, {{ $dbPort }}, {{ $dbName }}, {{ $dbUserName
}}, {{ $dbPasswordSecretName }}, and {{ $dbPasswordSecretKey }} with | quote
(including the JDBC URL parts so the full value: jdbc:mariadb://{{ $dbHost |
quote }}:{{ $dbPort | quote }}/{{ $dbName | quote }} and the DATABASE_USERNAME
and secret name/key entries) to ensure safe, quoted string rendering.
---
Outside diff comments:
In `@deploy/helm/grimmory/templates/deployment.yaml`:
- Around line 73-107: Update the documentation to reflect the deployment
template changes: document the new externalDatabase.* configuration fields
(externalDatabase.host/port/name/username/passwordSecretName/passwordSecretKey),
explain that the bundled DATABASE_URL default host now uses the release-scoped
host pattern (the template's $dbHost value, e.g. {release-name}-mariadb) and the
default port/name matching $dbPort/$dbName, and add a note that switching to
externalDatabase requires a pre-created Kubernetes Secret referenced by the
template's dbPasswordSecretName/dbPasswordSecretKey (used to populate
DATABASE_PASSWORD); also update installation.md, getting-started.md, and
testing-setup.md to show examples for both bundled and externalDatabase setups
and the corresponding environment variables DATABASE_URL, DATABASE_USERNAME and
DATABASE_PASSWORD.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 46aadf8f-77c7-40f9-b409-66d29c3ebd90
📒 Files selected for processing (3)
deploy/helm/grimmory/Chart.yamldeploy/helm/grimmory/templates/deployment.yamldeploy/helm/grimmory/values.yaml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: This project is being developed using current and future-facing technologies:
- Java 25 with --enable-preview (preview features are INTENTIONAL and encouraged)
- Spring Boot 4 (latest major version, check APIs accordingly)
- Jackson 3 (new package: tools.jackson.* instead of com.fasterxml.jackson.*)
- Hibernate 7.3.x (Jakarta Persistence 3.2, new APIs; avoid deprecated Hibernate 5/6 patterns)
- Angular 21 (signals-based reactivity, no NgModules unless legacy)
Grimmory Internal Tools
- epub4j and pdfium4j are our own internal tools developed by the Grimmory team.
- Always verify behavior and API changes against the upstream repositories:
- If you encounter issues with these libraries, check if a fix exists in the upstream grimmory-tools organization.
Metadata Standards and Compliance
- For all metadata writing and parsing logic, double-check against Dublin Core and ANSI standards to ensure perfect official compliance.
- We strictly follow the widespread and official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
General Java and Spring rules
- ALWAYS prefer modern, idiomatic Java 25 constructs over legacy patterns.
- Preview features (--enable-preview) are enabled and intentional; do NOT flag them as risky unless there is a concrete runtime issue.
- Prefer: records, sealed classes/interfaces, pattern matching (switch expressions, instanceof), structured concurrency (StructuredTaskScope), scoped values, string templates, unnamed patterns/variables.
- Prefer virtual threads (Thread.ofVirtual(), Executors.newVirtualThreadPerTaskExecutor()) over platform threads for I/O-bound work.
- Prefer the new Sequenced Collections API (SequencedCollection, SequencedMap) where applicable.
- Prefer
varfor local variables when the type is obvious from context.- Use stream().toList() instead of stream().collect(Collectors.toList()) for imm...
Files:
deploy/helm/grimmory/Chart.yamldeploy/helm/grimmory/templates/deployment.yamldeploy/helm/grimmory/values.yaml
🪛 Checkov (3.2.525)
deploy/helm/grimmory/templates/deployment.yaml
[low] 34-35: Base64 High Entropy String
(CKV_SECRET_6)
🪛 YAMLlint (1.38.0)
deploy/helm/grimmory/templates/deployment.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[error] 2-2: too many spaces inside braces
(braces)
[error] 3-3: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 5-5: too many spaces inside braces
(braces)
[error] 6-6: too many spaces inside braces
(braces)
[error] 18-18: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
[error] 20-20: too many spaces inside braces
(braces)
[error] 21-21: too many spaces inside braces
(braces)
[error] 22-22: too many spaces inside braces
(braces)
[error] 23-23: too many spaces inside braces
(braces)
[error] 24-24: too many spaces inside braces
(braces)
[error] 25-25: too many spaces inside braces
(braces)
[error] 26-26: too many spaces inside braces
(braces)
[error] 27-27: too many spaces inside braces
(braces)
[error] 28-28: too many spaces inside braces
(braces)
[error] 29-29: too many spaces inside braces
(braces)
[error] 30-30: too many spaces inside braces
(braces)
[error] 31-31: too many spaces inside braces
(braces)
[error] 32-32: too many spaces inside braces
(braces)
[error] 33-33: too many spaces inside braces
(braces)
[error] 34-34: too many spaces inside braces
(braces)
[error] 35-35: too many spaces inside braces
(braces)
[error] 36-36: too many spaces inside braces
(braces)
[error] 37-37: too many spaces inside braces
(braces)
[error] 38-38: too many spaces inside braces
(braces)
[error] 39-39: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 41-41: too many spaces inside braces
(braces)
[error] 42-42: too many spaces inside braces
(braces)
[error] 43-43: too many spaces inside braces
(braces)
[error] 44-44: too many spaces inside braces
(braces)
[error] 45-45: too many spaces inside braces
(braces)
🔀 Multi-repo context grimmory-tools/grimmory-docs
[::grimmory-tools/grimmory-docs::]
- docs/installation.md
- Lines mentioning default JDBC connection and host: "DATABASE_URL=jdbc:mariadb://mariadb:3306/grimmory" (around line 58). Also contains docker-compose mariadb service snippets and instructions to verify DATABASE_USERNAME / DATABASE_PASSWORD mapping (lines ~58–173).
- docs/getting-started.md
- References creating mariadb config directories and docker-compose usage (line ~21).
- docs/testing/testing-setup.md
- Contains a mariadb test container example and instructions referencing mariadb logs and config (lines ~49–85).
Relevance: documentation assumes a bundled MariaDB service reachable at host "mariadb" and uses DATABASE_URL jdbc form. The Helm change enabling externalDatabase and conditional mariadb subchart may require updating docs (installation/getting-started/testing) to document externalDatabase values and how DATABASE_URL / secrets should be set when using an external DB.
🔇 Additional comments (4)
deploy/helm/grimmory/values.yaml (1)
19-25: LGTM — standard Helm convention for optional configuration.The 2-space-indented comment style (
# host:) is idiomatic; users can uncomment each key while preserving correct YAML indentation relative to theexternalDatabase:parent.deploy/helm/grimmory/Chart.yaml (1)
26-26: LGTM — correct conditional dependency pattern.
condition: mariadb.enabledaligns with themariadb.enabled: truedefault invalues.yamland correctly gates subchart inclusion.deploy/helm/grimmory/templates/deployment.yaml (2)
1-6: LGTM — validation guards are correct and placed correctly.Both mutual-exclusivity and at-least-one checks use
{{- fail }}before the YAML document starts, ensuring clean render-time errors. Accessing.Values.externalDatabase.hoston a nil map is safe in Helm's Go template engine (missing map key returns nil,not nilistrue).
18-45: LGTM — variable scoping and secret-name derivation are correct.Go template
=assignment insideif/else/withblocks correctly mutates variables declared in the outer scope. The hardcoded"mariadb-password"key (line 34) andprintf "%s-mariadb" .Release.Namesecret name (line 33) are consistent with the cloudpirates/mariadb chart: the chart creates a secret named<release-name>-mariadb, and theuserPasswordKey: mariadb-passworddefault is corroborated by the commented example invalues.yaml(lines 13–15). The Checkov "Base64 High Entropy String" alert on lines 34–35 is a false positive —"mariadb-password"is a Kubernetes Secret key name, not a credential.
… the corresponding password key
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
deploy/helm/grimmory/templates/deployment.yaml (1)
100-107:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
| quoteon user-controlled string interpolations — not yet addressed from prior review.The JDBC URL,
DATABASE_USERNAME, and thesecretKeyRefname/keyfields are still rendered without the| quoteHelm filter. A username or database name containing#silently truncates the YAML value (YAML comment), and values containing:produce a mapping instead of a scalar, both of which would corrupt the Pod spec at apply-time with no obvious error.♻️ Proposed fix (aligns with prior review suggestion)
- value: jdbc:mariadb://{{ $dbHost }}:{{ $dbPort }}/{{ $dbName }} + value: {{ printf "jdbc:mariadb://%s:%v/%s" $dbHost $dbPort $dbName | quote }} - name: DATABASE_USERNAME - value: {{ $dbUserName }} + value: {{ $dbUserName | quote }} - name: DATABASE_PASSWORD valueFrom: secretKeyRef: - name: {{ $dbPasswordSecretName }} - key: {{ $dbPasswordSecretKey }} + name: {{ $dbPasswordSecretName | quote }} + key: {{ $dbPasswordSecretKey | quote }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/helm/grimmory/templates/deployment.yaml` around lines 100 - 107, The YAML interpolations in deployment.yaml are unquoted and can be corrupted by user-controlled characters; update the JDBC URL and the environment vars to use Helm's | quote filter: apply | quote to the entire jdbc string interpolation (jdbc:mariadb://{{ $dbHost }}:{{ $dbPort }}/{{ $dbName }}), to DATABASE_USERNAME ({{ $dbUserName }}), and to the secretKeyRef name/key ({{ $dbPasswordSecretName }}, {{ $dbPasswordSecretKey }}), so each rendered value is a safe YAML scalar and cannot be truncated or parsed as a mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/helm/grimmory/templates/deployment.yaml`:
- Around line 73-74: The rendered shell command in deployment.yaml uses unquoted
template values in the until nc invocation (the line with until nc -z -v -w5 {{
$dbHost }} {{ $dbPort }}), which can break if $dbHost contains spaces or
metacharacters; fix it by shell-quoting the interpolated values (e.g. wrap {{
$dbHost }} and preferably {{ $dbPort }} in quotes) so the nc arguments are
passed as single, safe tokens and the echo line that prints the host uses the
same quoted template.
---
Duplicate comments:
In `@deploy/helm/grimmory/templates/deployment.yaml`:
- Around line 100-107: The YAML interpolations in deployment.yaml are unquoted
and can be corrupted by user-controlled characters; update the JDBC URL and the
environment vars to use Helm's | quote filter: apply | quote to the entire jdbc
string interpolation (jdbc:mariadb://{{ $dbHost }}:{{ $dbPort }}/{{ $dbName }}),
to DATABASE_USERNAME ({{ $dbUserName }}), and to the secretKeyRef name/key ({{
$dbPasswordSecretName }}, {{ $dbPasswordSecretKey }}), so each rendered value is
a safe YAML scalar and cannot be truncated or parsed as a mapping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: be826a53-e192-4b27-a246-ed3df79caf46
📒 Files selected for processing (1)
deploy/helm/grimmory/templates/deployment.yaml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Frontend Lint Threshold Check
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: This project is being developed using current and future-facing technologies:
- Java 25 with --enable-preview (preview features are INTENTIONAL and encouraged)
- Spring Boot 4 (latest major version, check APIs accordingly)
- Jackson 3 (new package: tools.jackson.* instead of com.fasterxml.jackson.*)
- Hibernate 7.3.x (Jakarta Persistence 3.2, new APIs; avoid deprecated Hibernate 5/6 patterns)
- Angular 21 (signals-based reactivity, no NgModules unless legacy)
Grimmory Internal Tools
- epub4j and pdfium4j are our own internal tools developed by the Grimmory team.
- Always verify behavior and API changes against the upstream repositories:
- If you encounter issues with these libraries, check if a fix exists in the upstream grimmory-tools organization.
Metadata Standards and Compliance
- For all metadata writing and parsing logic, double-check against Dublin Core and ANSI standards to ensure perfect official compliance.
- We strictly follow the widespread and official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
General Java and Spring rules
- ALWAYS prefer modern, idiomatic Java 25 constructs over legacy patterns.
- Preview features (--enable-preview) are enabled and intentional; do NOT flag them as risky unless there is a concrete runtime issue.
- Prefer: records, sealed classes/interfaces, pattern matching (switch expressions, instanceof), structured concurrency (StructuredTaskScope), scoped values, string templates, unnamed patterns/variables.
- Prefer virtual threads (Thread.ofVirtual(), Executors.newVirtualThreadPerTaskExecutor()) over platform threads for I/O-bound work.
- Prefer the new Sequenced Collections API (SequencedCollection, SequencedMap) where applicable.
- Prefer
varfor local variables when the type is obvious from context.- Use stream().toList() instead of stream().collect(Collectors.toList()) for imm...
Files:
deploy/helm/grimmory/templates/deployment.yaml
🪛 Checkov (3.2.525)
deploy/helm/grimmory/templates/deployment.yaml
[low] 34-35: Base64 High Entropy String
(CKV_SECRET_6)
🪛 YAMLlint (1.38.0)
deploy/helm/grimmory/templates/deployment.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[error] 2-2: too many spaces inside braces
(braces)
[error] 3-3: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 5-5: too many spaces inside braces
(braces)
[error] 6-6: too many spaces inside braces
(braces)
[error] 18-18: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
[error] 20-20: too many spaces inside braces
(braces)
[error] 21-21: too many spaces inside braces
(braces)
[error] 22-22: too many spaces inside braces
(braces)
[error] 23-23: too many spaces inside braces
(braces)
[error] 24-24: too many spaces inside braces
(braces)
[error] 25-25: too many spaces inside braces
(braces)
[error] 26-26: too many spaces inside braces
(braces)
[error] 27-27: too many spaces inside braces
(braces)
[error] 28-28: too many spaces inside braces
(braces)
[error] 29-29: too many spaces inside braces
(braces)
[error] 30-30: too many spaces inside braces
(braces)
[error] 31-31: too many spaces inside braces
(braces)
[error] 32-32: too many spaces inside braces
(braces)
[error] 33-33: too many spaces inside braces
(braces)
[error] 34-34: too many spaces inside braces
(braces)
[error] 35-35: too many spaces inside braces
(braces)
[error] 36-36: too many spaces inside braces
(braces)
[error] 37-37: too many spaces inside braces
(braces)
[error] 38-38: too many spaces inside braces
(braces)
[error] 39-39: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 41-41: too many spaces inside braces
(braces)
[error] 42-42: too many spaces inside braces
(braces)
[error] 43-43: too many spaces inside braces
(braces)
[error] 44-44: too many spaces inside braces
(braces)
[error] 45-45: too many spaces inside braces
(braces)
🔀 Multi-repo context grimmory-tools/grimmory-docs
[::grimmory-tools/grimmory-docs::]
- docs/installation.md
- References default DB host and JDBC URL: "DATABASE_URL=jdbc:mariadb://mariadb:3306/grimmory" and docker-compose examples that set DATABASE_URL, DATABASE_USERNAME, DATABASE_PASSWORD and depend on a service named mariadb (see env/block and depends_on). (see occurrences around the env example and docker-compose snippet)
- Notes verifying DATABASE_PASSWORD matches MYSQL_PASSWORD and DATABASE_USERNAME matches MYSQL_USER (db credential mapping guidance).
- docs/getting-started.md
- Creates mariadb config dirs and references bundled mariadb service (mkdir -p ~/grimmory/{mariadb/config,...}).
- docs/testing/testing-setup.md
- Contains mariadb test container example and instructions referencing mariadb logs/config and healthchecks.
Summary: Documentation currently assumes a bundled MariaDB reachable at host "mariadb" and examples use JDBC DATABASE_URL patterns. The Helm PR introducing externalDatabase and conditional mariadb subchart can break or confuse users following these docs unless the docs are updated to show how to configure DATABASE_URL/DATABASE_USERNAME/DATABASE_PASSWORD and secrets when using an external database (host, port, secret/key, and disabling the bundled subchart).
🔇 Additional comments (2)
deploy/helm/grimmory/templates/deployment.yaml (2)
1-6: Validation guards are correctly implemented.Both the mutual-exclusivity check (
mariadb.enabled+externalDatabase.host) and the at-least-one-must-be-set check are logically sound. The prior concern about the missingrequiredguard onmariadb.auth.secretKeys.userPasswordKey(line 31) is also addressed in the current revision.
25-26: No fix needed —mariadb.service.portis explicitly declared in values.yaml.The parent chart's
values.yaml(line 17) already definesmariadb.service.port: 3306, so.Values.mariadb.service.portwill never be nil whenmariadb.enabledis true. The proposed| default 3306fallback is unnecessary.Line 39's
| default 3306on the external database path serves a different purpose:externalDatabaseis optional and its structure may not exist at all if the external database is not configured. The bundled MariaDB case is already backed by an explicit default in the chart's values, making an additional filter redundant.> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/helm/grimmory/templates/deployment.yaml (1)
67-78:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
wait-for-databaseonly proves the port is open.This initContainer never verifies MariaDB authentication or that the target database is usable. With an external DB,
nc -zcan succeed while the app still fails immediately on bad credentials, missing schema, or a server that is listening but not yet ready for SQL. That falls short of the linked objective to ensure the pod can connect before startup. Use a MariaDB client image and run a lightweight authenticated probe such asmariadb-admin pingorSELECT 1with the same host/user/password/database inputs wired into the main container.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/helm/grimmory/templates/deployment.yaml` around lines 67 - 78, The initContainer named "wait-for-database" currently uses busybox and nc (nc -z -v -w5 {{ $dbHost }} {{ $dbPort }}) which only verifies the port is open; replace it with a MariaDB client image (e.g., mariadb or mysql client) and perform an authenticated probe using the same credentials and DB wiring as the main container—e.g., run mariadb-admin ping or execute a lightweight SQL like SELECT 1 against {{ $dbHost }}:{{ $dbPort }} with the configured user/password/database; update the container image, command, and env references so the probe fails if authentication, schema, or SQL readiness is not satisfied (targeting the "wait-for-database" initContainer and removing the nc-based loop).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@deploy/helm/grimmory/templates/deployment.yaml`:
- Around line 67-78: The initContainer named "wait-for-database" currently uses
busybox and nc (nc -z -v -w5 {{ $dbHost }} {{ $dbPort }}) which only verifies
the port is open; replace it with a MariaDB client image (e.g., mariadb or mysql
client) and perform an authenticated probe using the same credentials and DB
wiring as the main container—e.g., run mariadb-admin ping or execute a
lightweight SQL like SELECT 1 against {{ $dbHost }}:{{ $dbPort }} with the
configured user/password/database; update the container image, command, and env
references so the probe fails if authentication, schema, or SQL readiness is not
satisfied (targeting the "wait-for-database" initContainer and removing the
nc-based loop).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 185b2847-8f50-459a-8af8-49e7b6d29b5e
📒 Files selected for processing (1)
deploy/helm/grimmory/templates/deployment.yaml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Frontend Lint Threshold Check
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: This project is being developed using current and future-facing technologies:
- Java 25 with --enable-preview (preview features are INTENTIONAL and encouraged)
- Spring Boot 4 (latest major version, check APIs accordingly)
- Jackson 3 (new package: tools.jackson.* instead of com.fasterxml.jackson.*)
- Hibernate 7.3.x (Jakarta Persistence 3.2, new APIs; avoid deprecated Hibernate 5/6 patterns)
- Angular 21 (signals-based reactivity, no NgModules unless legacy)
Grimmory Internal Tools
- epub4j and pdfium4j are our own internal tools developed by the Grimmory team.
- Always verify behavior and API changes against the upstream repositories:
- If you encounter issues with these libraries, check if a fix exists in the upstream grimmory-tools organization.
Metadata Standards and Compliance
- For all metadata writing and parsing logic, double-check against Dublin Core and ANSI standards to ensure perfect official compliance.
- We strictly follow the widespread and official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
General Java and Spring rules
- ALWAYS prefer modern, idiomatic Java 25 constructs over legacy patterns.
- Preview features (--enable-preview) are enabled and intentional; do NOT flag them as risky unless there is a concrete runtime issue.
- Prefer: records, sealed classes/interfaces, pattern matching (switch expressions, instanceof), structured concurrency (StructuredTaskScope), scoped values, string templates, unnamed patterns/variables.
- Prefer virtual threads (Thread.ofVirtual(), Executors.newVirtualThreadPerTaskExecutor()) over platform threads for I/O-bound work.
- Prefer the new Sequenced Collections API (SequencedCollection, SequencedMap) where applicable.
- Prefer
varfor local variables when the type is obvious from context.- Use stream().toList() instead of stream().collect(Collectors.toList()) for imm...
Files:
deploy/helm/grimmory/templates/deployment.yaml
🪛 Checkov (3.2.525)
deploy/helm/grimmory/templates/deployment.yaml
[low] 34-35: Base64 High Entropy String
(CKV_SECRET_6)
🪛 YAMLlint (1.38.0)
deploy/helm/grimmory/templates/deployment.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[error] 2-2: too many spaces inside braces
(braces)
[error] 3-3: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 5-5: too many spaces inside braces
(braces)
[error] 6-6: too many spaces inside braces
(braces)
[error] 18-18: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
[error] 20-20: too many spaces inside braces
(braces)
[error] 21-21: too many spaces inside braces
(braces)
[error] 22-22: too many spaces inside braces
(braces)
[error] 23-23: too many spaces inside braces
(braces)
[error] 24-24: too many spaces inside braces
(braces)
[error] 25-25: too many spaces inside braces
(braces)
[error] 26-26: too many spaces inside braces
(braces)
[error] 27-27: too many spaces inside braces
(braces)
[error] 28-28: too many spaces inside braces
(braces)
[error] 29-29: too many spaces inside braces
(braces)
[error] 30-30: too many spaces inside braces
(braces)
[error] 31-31: too many spaces inside braces
(braces)
[error] 32-32: too many spaces inside braces
(braces)
[error] 33-33: too many spaces inside braces
(braces)
[error] 34-34: too many spaces inside braces
(braces)
[error] 35-35: too many spaces inside braces
(braces)
[error] 36-36: too many spaces inside braces
(braces)
[error] 37-37: too many spaces inside braces
(braces)
[error] 38-38: too many spaces inside braces
(braces)
[error] 39-39: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 41-41: too many spaces inside braces
(braces)
[error] 42-42: too many spaces inside braces
(braces)
[error] 43-43: too many spaces inside braces
(braces)
[error] 44-44: too many spaces inside braces
(braces)
[error] 45-45: too many spaces inside braces
(braces)
🔀 Multi-repo context grimmory-tools/grimmory-docs
[::grimmory-tools/grimmory-docs::]
- docs/installation.md
- Provides docker-compose and .env examples that hardcode DATABASE_URL=jdbc:mariadb://mariadb:3306/grimmory and show env vars DATABASE_USERNAME/DATABASE_PASSWORD mapping to container MYSQL_USER/MYSQL_PASSWORD. (occurrences around the .env snippet and docker-compose example)
- Notes and troubleshooting explicitly instruct verifying DATABASE_PASSWORD matches MYSQL_PASSWORD and DATABASE_USERNAME matches MYSQL_USER.
- docs/getting-started.md
- Setup instructions create directories ~/grimmory/{mariadb/...} and refer readers to installation guide using a bundled mariadb service.
- docs/testing/testing-setup.md
- Local test compose examples name the mariadb service container_name mariadb-test and reference checking mariadb logs/health.
Implication: documentation and local Docker Compose examples assume a bundled MariaDB reachable at host "mariadb" and specific env/secret names. The Helm PR adding externalDatabase and conditional mariadb subchart will require updating docs/installation and related guides to show how to configure DATABASE_URL, DATABASE_USERNAME, DATABASE_PASSWORD and secrets when using an external DB (host/port/secret key) and to document disabling the bundled mariadb subchart.
🔇 Additional comments (1)
deploy/helm/grimmory/templates/deployment.yaml (1)
1-6: Consider a defensive default forexternalDatabaseas a best practice for consistency, but render will succeed as-is.The validation guards at lines 1–6 access
.Values.externalDatabase.hostdirectly. WhenexternalDatabaseis omitted (the default), Go templates safely evaluate.Values.externalDatabaseasnil, makingand true nilresolve tofalse. The conditions are skipped, and the template renders without error. The later pattern at line 37 useswith .Values.externalDatabasefor safe guard; mirroring that defensive style in lines 1–6 would improve consistency and readability, but is not required to prevent failure.Optional consistency pattern
+{{- $externalDatabase := .Values.externalDatabase | default dict }} -{{- if and .Values.mariadb.enabled .Values.externalDatabase.host }} +{{- if and .Values.mariadb.enabled $externalDatabase.host }} {{- fail "mariadb.enabled and externalDatabase are mutually exclusive: disable the bundled MariaDB subchart or remove the externalDatabase" }} {{- end }} -{{- if and (not .Values.mariadb.enabled) (not .Values.externalDatabase.host) }} +{{- if and (not .Values.mariadb.enabled) (not $externalDatabase.host) }} {{- fail "At least one database must be set" }} {{- end }}> Likely an incorrect or invalid review comment.
|
The issue raised by coderabbit about the sketchy way to wait for the db to be up is relevant and that's something I can fix, but I think it's worth having a distinct PR for. If someone wants to create an issue for it I can send a PR for it too. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/helm/grimmory/templates/deployment.yaml`:
- Around line 1-5: Update the docs to reflect the new external DB install
contract: in docs/installation.md, docs/getting-started.md, and
docs/testing/testing-setup.md replace bundled-only instructions and hardcoded
JDBC examples like "jdbc:mariadb://mariadb:3306/grimmory" with guidance for
using externalDatabase.* values (.Values.externalDatabase.host,
.Values.externalDatabase.port, .Values.externalDatabase.user,
.Values.externalDatabase.password, .Values.externalDatabase.database), document
the mutual-exclusion rule with .Values.mariadb.enabled, show the correct
templated JDBC/url format for external hosts, and add a note about configuring
TLS/auth if applicable; apply the same doc updates for any other template
sections that reference the bundled MariaDB (the other block mentioned in the
templates).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 777de755-f4ff-4ea3-a1fa-3b146711c755
📒 Files selected for processing (1)
deploy/helm/grimmory/templates/deployment.yaml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Frontend Lint Threshold Check
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: This project is being developed using current and future-facing technologies:
- Java 25 with --enable-preview (preview features are INTENTIONAL and encouraged)
- Spring Boot 4 (latest major version, check APIs accordingly)
- Jackson 3 (new package: tools.jackson.* instead of com.fasterxml.jackson.*)
- Hibernate 7.3.x (Jakarta Persistence 3.2, new APIs; avoid deprecated Hibernate 5/6 patterns)
- Angular 21 (signals-based reactivity, no NgModules unless legacy)
Grimmory Internal Tools
- epub4j and pdfium4j are our own internal tools developed by the Grimmory team.
- Always verify behavior and API changes against the upstream repositories:
- If you encounter issues with these libraries, check if a fix exists in the upstream grimmory-tools organization.
Metadata Standards and Compliance
- For all metadata writing and parsing logic, double-check against Dublin Core and ANSI standards to ensure perfect official compliance.
- We strictly follow the widespread and official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
General Java and Spring rules
- ALWAYS prefer modern, idiomatic Java 25 constructs over legacy patterns.
- Preview features (--enable-preview) are enabled and intentional; do NOT flag them as risky unless there is a concrete runtime issue.
- Prefer: records, sealed classes/interfaces, pattern matching (switch expressions, instanceof), structured concurrency (StructuredTaskScope), scoped values, string templates, unnamed patterns/variables.
- Prefer virtual threads (Thread.ofVirtual(), Executors.newVirtualThreadPerTaskExecutor()) over platform threads for I/O-bound work.
- Prefer the new Sequenced Collections API (SequencedCollection, SequencedMap) where applicable.
- Prefer
varfor local variables when the type is obvious from context.- Use stream().toList() instead of stream().collect(Collectors.toList()) for imm...
Files:
deploy/helm/grimmory/templates/deployment.yaml
🪛 Checkov (3.2.525)
deploy/helm/grimmory/templates/deployment.yaml
[low] 34-35: Base64 High Entropy String
(CKV_SECRET_6)
🪛 YAMLlint (1.38.0)
deploy/helm/grimmory/templates/deployment.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[error] 2-2: too many spaces inside braces
(braces)
[error] 3-3: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 5-5: too many spaces inside braces
(braces)
[error] 6-6: too many spaces inside braces
(braces)
[error] 18-18: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
[error] 20-20: too many spaces inside braces
(braces)
[error] 21-21: too many spaces inside braces
(braces)
[error] 22-22: too many spaces inside braces
(braces)
[error] 23-23: too many spaces inside braces
(braces)
[error] 24-24: too many spaces inside braces
(braces)
[error] 25-25: too many spaces inside braces
(braces)
[error] 26-26: too many spaces inside braces
(braces)
[error] 27-27: too many spaces inside braces
(braces)
[error] 28-28: too many spaces inside braces
(braces)
[error] 29-29: too many spaces inside braces
(braces)
[error] 30-30: too many spaces inside braces
(braces)
[error] 31-31: too many spaces inside braces
(braces)
[error] 32-32: too many spaces inside braces
(braces)
[error] 33-33: too many spaces inside braces
(braces)
[error] 34-34: too many spaces inside braces
(braces)
[error] 35-35: too many spaces inside braces
(braces)
[error] 36-36: too many spaces inside braces
(braces)
[error] 37-37: too many spaces inside braces
(braces)
[error] 38-38: too many spaces inside braces
(braces)
[error] 39-39: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 41-41: too many spaces inside braces
(braces)
[error] 42-42: too many spaces inside braces
(braces)
[error] 43-43: too many spaces inside braces
(braces)
[error] 44-44: too many spaces inside braces
(braces)
[error] 45-45: too many spaces inside braces
(braces)
🔀 Multi-repo context grimmory-tools/grimmory-docs
[::grimmory-tools/grimmory-docs::]
-
docs/installation.md
- Multiple places hardcode or document a bundled MariaDB and env var mappings that assume host "mariadb" and specific secret/var names:
- DATABASE_URL example: "DATABASE_URL=jdbc:mariadb://mariadb:3306/grimmory" (line showing DATABASE_URL at docs/installation.md:58)
- Environment mapping in docker-compose snippet: DATABASE_URL, DATABASE_USERNAME, DATABASE_PASSWORD (around docs/installation.md lines ~84–86)
- MariaDB service block with container_name mariadb and MYSQL_* envs (around docs/installation.md lines ~99–114)
- Notes: "DB_PASSWORD must match ... MYSQL_PASSWORD" and "Ensure DATABASE_PASSWORD matches MYSQL_PASSWORD and DATABASE_USERNAME matches MYSQL_USER." (docs/installation.md lines ~123 and ~168)
- Implication: Helm change allowing externalDatabase + conditional mariadb subchart will require documentation updates to show how to set DATABASE_URL/DATABASE_USERNAME/DATABASE_PASSWORD and secrets when using an external DB.
- Multiple places hardcode or document a bundled MariaDB and env var mappings that assume host "mariadb" and specific secret/var names:
-
docs/getting-started.md
- Setup steps create directories referencing mariadb (mkdir -p ~/grimmory/{mariadb/config,...}) and instruct using bundled MariaDB in examples (docs/getting-started.md:21 and surrounding content).
- Implication: Getting-started should include instructions for configuring Grimmory to use an external DB and omitting bundled mariadb.
-
docs/testing/testing-setup.md
- Test docker-compose examples refer to container_name mariadb-test and commands to check mariadb logs/health (lines ~49–75, around "docker compose logs mariadb" and health/reset steps).
- Implication: Test docs assume a local bundled DB; may need examples for testing with external DB (e.g., pointing DATABASE_URL to external host, or skipping mariadb service).
Conclusion: The docs repository contains multiple references and examples that assume a bundled MariaDB reachable at host "mariadb" and specific env/secret names. The helm PR adding externalDatabase support and conditional mariadb subchart is likely to require updates to installation/getting-started/testing docs to document external DB configuration, secret naming/key mapping, and disabling the bundled subchart.
🔇 Additional comments (1)
deploy/helm/grimmory/templates/deployment.yaml (1)
1-5: Good fail-fast DB backend validation.The
fail/requiredguards make ambiguous or incomplete database configuration break at render time instead of surfacing later as a broken Pod.Also applies to: 37-43
| {{- if and .Values.mariadb.enabled .Values.externalDatabase.host }} | ||
| {{- fail "mariadb.enabled and externalDatabase are mutually exclusive: disable the bundled MariaDB subchart or remove the externalDatabase" }} | ||
| {{- end }} | ||
| {{- if and (not .Values.mariadb.enabled) (not .Values.externalDatabase.host) }} | ||
| {{- fail "At least one database must be set" }} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Update the linked docs for external DB installs.
This changes the install contract, but grimmory-tools/grimmory-docs still documents bundled-MariaDB-only flows and hardcoded jdbc:mariadb://mariadb:3306/grimmory examples. A companion update in docs/installation.md, docs/getting-started.md, and docs/testing/testing-setup.md would prevent copy/paste misconfiguration for externalDatabase.*.
Also applies to: 99-107
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[error] 2-2: too many spaces inside braces
(braces)
[error] 3-3: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 5-5: too many spaces inside braces
(braces)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/helm/grimmory/templates/deployment.yaml` around lines 1 - 5, Update
the docs to reflect the new external DB install contract: in
docs/installation.md, docs/getting-started.md, and docs/testing/testing-setup.md
replace bundled-only instructions and hardcoded JDBC examples like
"jdbc:mariadb://mariadb:3306/grimmory" with guidance for using
externalDatabase.* values (.Values.externalDatabase.host,
.Values.externalDatabase.port, .Values.externalDatabase.user,
.Values.externalDatabase.password, .Values.externalDatabase.database), document
the mutual-exclusion rule with .Values.mariadb.enabled, show the correct
templated JDBC/url format for external hosts, and add a note about configuring
TLS/auth if applicable; apply the same doc updates for any other template
sections that reference the bundled MariaDB (the other block mentioned in the
templates).
There was a problem hiding this comment.
This seems to be about the docker compose docs. It would make sense to eventually have a section about the helm chart, but this is beyond the scope of this PR.
There was a problem hiding this comment.
@thibaultamartin, that's fair — the existing docs are docker-compose-focused and a dedicated Helm section is a separate effort. Would you like me to open a follow-up issue in grimmory-tools/grimmory-docs (or grimmory-tools/grimmory) to track adding Helm chart documentation covering the externalDatabase.* values and the mutual-exclusion rule with mariadb.enabled?
Move database connection logic from brittle Go variables to helpers, so assignment is done in a separate file and deployment only focuses on using it.
1 participant
Description
This PR adds support for an external MariaDB database.
Linked Issue: Fixes #1059
Changes
externalDatabseblock in the values fileSummary by CodeRabbit
New Features
Bug Fixes / Validation