Make db type configurable with env variable#32
Conversation
WalkthroughThe pull request updates database configuration to use an environment variable placeholder for the database type instead of hardcoding database driver values. This involves updates to both documentation and deployment configuration files. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
211-227:⚠️ Potential issue | 🟡 MinorFix inconsistency between instruction text and code example.
Line 211 states "set
type: postgres" but the example on line 216 showstype: ${OPENFGC_DB_TYPE}. This inconsistency could confuse users about whether they should:
- Set the value directly to
postgres, or- Set the
OPENFGC_DB_TYPEenvironment variable topostgresProposed fix for consistency
-For PostgreSQL, set `type: postgres` and use the default port `5432`: +For PostgreSQL, set the `OPENFGC_DB_TYPE` environment variable to `postgres` and use the default port `5432`:Or alternatively, update the example to be more explicit:
-For PostgreSQL, set `type: postgres` and use the default port `5432`: +For PostgreSQL configuration with environment variables:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 211 - 227, The README text saying "set `type: postgres`" is inconsistent with the YAML sample using `type: ${OPENFGC_DB_TYPE}`; pick one approach and make them consistent—either change the prose to instruct users to set the OPENFGC_DB_TYPE environment variable to "postgres" (and keep `type: ${OPENFGC_DB_TYPE}` in the sample) or change the YAML example to `type: postgres` (and leave the prose as-is); update the line containing `type: ${OPENFGC_DB_TYPE}` (and the surrounding explanation) accordingly so the instruction and example match.
🧹 Nitpick comments (3)
README.md (3)
231-238: Consider adding example values for completeness.The table now includes the
OPENFGC_DB_TYPEvariable with example values, which is good. However, theOPENFGC_DB_NAME,OPENFGC_DB_USER, andOPENFGC_DB_PASSWORDrows have empty example values. While this might be intentional to avoid suggesting insecure defaults, adding generic examples could help users understand the expected format.Optional: Add example values
| Variable | Description | Example Values | |----------|-------------|---------| | `OPENFGC_DB_TYPE` | Database type | `mysql`, `sqlite`, `postgres` | | `OPENFGC_DB_HOSTNAME` | Database hostname | `localhost` | | `OPENFGC_DB_PORT` | Database port | `3306` for MySQL, `5432` for PostgreSQL | -| `OPENFGC_DB_NAME` | Database name | | -| `OPENFGC_DB_USER` | Database user | | -| `OPENFGC_DB_PASSWORD` | Database password | | +| `OPENFGC_DB_NAME` | Database name | `consent_mgt` | +| `OPENFGC_DB_USER` | Database user | `admin` | +| `OPENFGC_DB_PASSWORD` | Database password | (set securely) |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 231 - 238, Add safe, generic example values to the OPENFGC_DB_NAME, OPENFGC_DB_USER, and OPENFGC_DB_PASSWORD table rows so users see the expected format; for example use a neutral DB name like "app_db" for OPENFGC_DB_NAME, a non-privileged username like "app_user" for OPENFGC_DB_USER, and a placeholder password format like "secure_password" or "<REDACTED_PASSWORD>" for OPENFGC_DB_PASSWORD to indicate a secret value rather than a real password.
229-229: Clarify what "change the configuration file directly" means.The text states users can "change the configuration file directly in
deployment.yaml" but doesn't explicitly clarify that this means replacing the${...}placeholders with actual values. Since the default file now contains placeholders, this could be ambiguous.Suggested improvement
-Either change the configuration file directly in `deployment.yaml` or set the following environment variables before starting the server: +Either replace the `${...}` placeholders in `deployment.yaml` with actual values, or set the following environment variables before starting the server:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 229, Clarify the README line about editing deployment.yaml by explicitly stating that users should replace the placeholder tokens (e.g., ${...} entries) in deployment.yaml with their actual values or secrets; update the sentence around "change the configuration file directly in `deployment.yaml`" to say "open deployment.yaml and replace the `${...}` placeholder(s) with your actual configuration values" and mention that alternatively they can export the equivalent environment variables before starting the server.
185-209: Clarify configuration approach earlier in the documentation.The example shows environment variable placeholders (
${OPENFGC_DB_TYPE}) but the preceding text "Update configuration file at..." might lead users to think they should replace these with actual values. While line 229 later explains that users can either modify the file directly or use environment variables, introducing this concept here would improve clarity.Suggested improvement
Consider adding a brief note before the configuration example:
### 3. Configure Application -Update configuration file at `target/server/repository/conf/deployment.yaml`: +Configure the application by either: +1. Setting the environment variables shown in the `${...}` placeholders below, or +2. Replacing the placeholders directly in `target/server/repository/conf/deployment.yaml` with actual values. + +Configuration file location: `target/server/repository/conf/deployment.yaml`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 185 - 209, Add a brief clarifying sentence immediately before the YAML configuration example that explains the placeholders (e.g. ${OPENFGC_DB_TYPE}, ${OPENFGC_DB_HOSTNAME}, ${OPENFGC_DB_PORT}, ${OPENFGC_DB_NAME}, ${OPENFGC_DB_USER}, ${OPENFGC_DB_PASSWORD}) are environment-variable substitutions and that users can either edit the shown deployment.yaml config file directly or leave the placeholders and set the corresponding environment variables instead; keep the note concise and place it just above the YAML block so readers understand the two configuration options up front.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@consent-server/cmd/server/repository/conf/deployment.yaml`:
- Line 10: The deployment.yaml currently substitutes type: ${OPENFGC_DB_TYPE}
which yields an empty string when unset; update the template to provide an
explicit default (e.g., use your templating syntax to set OPENFGC_DB_TYPE to
mysql when unset) and/or add a fail-fast check in the config validation (in
validateConfig / config.go referencing config.Database.Consent.Type) to return
an error if the env var is missing, so the runtime behavior is explicit and not
implicitly falling back to mysql.
---
Outside diff comments:
In `@README.md`:
- Around line 211-227: The README text saying "set `type: postgres`" is
inconsistent with the YAML sample using `type: ${OPENFGC_DB_TYPE}`; pick one
approach and make them consistent—either change the prose to instruct users to
set the OPENFGC_DB_TYPE environment variable to "postgres" (and keep `type:
${OPENFGC_DB_TYPE}` in the sample) or change the YAML example to `type:
postgres` (and leave the prose as-is); update the line containing `type:
${OPENFGC_DB_TYPE}` (and the surrounding explanation) accordingly so the
instruction and example match.
---
Nitpick comments:
In `@README.md`:
- Around line 231-238: Add safe, generic example values to the OPENFGC_DB_NAME,
OPENFGC_DB_USER, and OPENFGC_DB_PASSWORD table rows so users see the expected
format; for example use a neutral DB name like "app_db" for OPENFGC_DB_NAME, a
non-privileged username like "app_user" for OPENFGC_DB_USER, and a placeholder
password format like "secure_password" or "<REDACTED_PASSWORD>" for
OPENFGC_DB_PASSWORD to indicate a secret value rather than a real password.
- Line 229: Clarify the README line about editing deployment.yaml by explicitly
stating that users should replace the placeholder tokens (e.g., ${...} entries)
in deployment.yaml with their actual values or secrets; update the sentence
around "change the configuration file directly in `deployment.yaml`" to say
"open deployment.yaml and replace the `${...}` placeholder(s) with your actual
configuration values" and mention that alternatively they can export the
equivalent environment variables before starting the server.
- Around line 185-209: Add a brief clarifying sentence immediately before the
YAML configuration example that explains the placeholders (e.g.
${OPENFGC_DB_TYPE}, ${OPENFGC_DB_HOSTNAME}, ${OPENFGC_DB_PORT},
${OPENFGC_DB_NAME}, ${OPENFGC_DB_USER}, ${OPENFGC_DB_PASSWORD}) are
environment-variable substitutions and that users can either edit the shown
deployment.yaml config file directly or leave the placeholders and set the
corresponding environment variables instead; keep the note concise and place it
just above the YAML block so readers understand the two configuration options up
front.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e09d2f7c-e020-441d-8d2b-4eef0616e5f6
📒 Files selected for processing (2)
README.mdconsent-server/cmd/server/repository/conf/deployment.yaml
| database: | ||
| consent: | ||
| type: mysql | ||
| type: ${OPENFGC_DB_TYPE} |
There was a problem hiding this comment.
Consider documenting the default behavior when OPENFGC_DB_TYPE is unset.
If the OPENFGC_DB_TYPE environment variable is not set, the substitution will result in an empty string. Based on the validation logic in config.go (lines 273-292), an empty or unrecognized value defaults to mysql. This implicit fallback may not be obvious to users and could lead to unexpected behavior.
Consider one of the following approaches:
- Document the default behavior prominently in the README
- Add explicit validation to fail fast if the variable is unset
- Provide a default value in the YAML (e.g.,
type: ${OPENFGC_DB_TYPE:mysql}) if your templating system supports it
Alternative: Add validation for required environment variable
If you prefer explicit validation, you could modify the validation function to reject empty database types:
// In validateConfig()
if config.Database.Consent.Type == "" {
return fmt.Errorf("database type is required (set OPENFGC_DB_TYPE environment variable)")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@consent-server/cmd/server/repository/conf/deployment.yaml` at line 10, The
deployment.yaml currently substitutes type: ${OPENFGC_DB_TYPE} which yields an
empty string when unset; update the template to provide an explicit default
(e.g., use your templating syntax to set OPENFGC_DB_TYPE to mysql when unset)
and/or add a fail-fast check in the config validation (in validateConfig /
config.go referencing config.Database.Consent.Type) to return an error if the
env var is missing, so the runtime behavior is explicit and not implicitly
falling back to mysql.
2 participants
$subject
Summary by CodeRabbit
Documentation
OPENFGC_DB_TYPEparameter and example values.Chores