External database: unmanaged mode, custom Secret key, postgresql://#456
Open
jalet wants to merge 2 commits into
Open
External database: unmanaged mode, custom Secret key, postgresql://#456jalet wants to merge 2 commits into
jalet wants to merge 2 commits into
Conversation
mm-cloud-bot
added
the
kind/feature
|
@jalet: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsI understand the commands that are listed here |
mm-cloud-bot
added
do-not-merge/release-note-label-needed
release-note
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughAdds Database.Unmanaged and ExternalDatabase.ConnectionStringKey, makes DB utility methods, controller, and deployment generator skip operator-managed DB behavior when unmanaged, extends external DB handling for configurable secret key and postgresql://, and adds tests across the stack. ChangesUnmanaged DB + ExternalKey
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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)
config/crd/bases/installation.mattermost.com_mattermosts.yaml (1)
104-121:⚠️ Potential issue | 🟡 MinorUpdate the Secret description to include custom keys.
Line 117 still says the Secret should contain
DB_CONNECTION_STRING, but withconnectionStringKeya Secret containing onlyuriis valid.📝 Proposed documentation update
secret: description: |- Secret contains data necessary to connect to the external database. The Kubernetes Secret should contain: - - Key: DB_CONNECTION_STRING | Value: Full database connection string. + - Key: DB_CONNECTION_STRING, or the key configured by connectionStringKey | Value: Full database connection string. It can also contain optional fields, such as:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/crd/bases/installation.mattermost.com_mattermosts.yaml` around lines 104 - 121, Update the Secret description under the "secret" field to reflect that the key for the connection string is configurable via connectionStringKey: state that the default key is "DB_CONNECTION_STRING" but if connectionStringKey is set the operator will use that key (for example "uri"), and that the operator will also fall back to that key for MM_SQLSETTINGS_DATASOURCE when no MM_SQLSETTINGS_DATASOURCE entry exists in the Secret; keep the existing optional keys (MM_SQLSETTINGS_DATASOURCEREPLICAS, DB_CONNECTION_CHECK_URL) unchanged.
♻️ Duplicate comments (1)
apis/mattermost/v1alpha1/zz_generated.openapi.go (1)
20-338:⚠️ Potential issue | 🟠 MajorSame unrelated path-format rewrite as v1beta1; please regenerate both together.
Every change in this file is the same class of rewrite as in
apis/mattermost/v1beta1/zz_generated.openapi.go: schema map keys, factory function names,ref(...)targets, andDependenciesentries moved fromgitlite.zycloud.tk/mattermost/mattermost-operator/apis/mattermost/v1alpha1.*to./apis/mattermost/v1alpha1.*, with theschema__apis_mattermost_v1alpha1_*double-underscore form indicating an empty module component.
v1alpha1holds the deprecatedClusterInstallationAPI and is not part of this PR's feature scope (unmanaged DB /ConnectionStringKey/postgresql://all live onv1beta1.Mattermost), so there is no functional reason for this file to have changed. See the detailed review on the v1beta1 file for the full concern about consumer impact and next-make generatethrash — the same remediation applies here: regenerate both files from an environment that produces the previous fully-qualified module-path keys, or make the rename explicit and intentional across both packages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/mattermost/v1alpha1/zz_generated.openapi.go` around lines 20 - 338, Generated openapi keys and refs in zz_generated.openapi.go for package v1alpha1 were rewritten to relative paths (./apis/mattermost/v1alpha1.*) and double-underscore schema function names (schema__apis_mattermost_v1alpha1_*), matching the same unwanted change made in v1beta1; restore the original fully-qualified module-path keys and factory names by regenerating the OpenAPI files for both v1alpha1 and v1beta1 together from the correct generator environment or reverting the changes: ensure functions like schema__apis_mattermost_v1alpha1_ClusterInstallation (and its peers), the ref(...) targets, and the Dependencies entries use the original github.com/mattermost/mattermost-operator/apis/mattermost/v1alpha1.* fully-qualified references instead of ./apis/... so both generated files are consistent and prevent make generate churn.
🧹 Nitpick comments (1)
pkg/mattermost/mattermost_v1beta_test.go (1)
1125-1127: Exercise the unmanaged guard with a non-nil DB config.Passing
nilmeans this test would still pass ifGenerateDeploymentV1BetaignoredDatabase.Unmanagedand only skipped DB injection becausedb == nil.🧪 Proposed test strengthening
- // Passing a nil DatabaseConfig mirrors checkDatabase returning nil for unmanaged. - deployment := GenerateDeploymentV1Beta(mattermost, nil, fileStore, "mm", "", "sa", "") + dbConfig := &ExternalDBConfig{ + secretName: "db-secret", + connectionStringKey: DefaultExternalDBConnectionStringKey, + hasDBCheckURL: true, + dbType: database.PostgreSQLDatabase, + } + deployment := GenerateDeploymentV1Beta(mattermost, dbConfig, fileStore, "mm", "", "sa", "")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/mattermost/mattermost_v1beta_test.go` around lines 1125 - 1127, The test currently passes nil for the DB config so it can't verify the unmanaged guard; instead construct and pass a non-nil database config with Unmanaged set true (e.g., a DatabaseConfig/Database struct instance with Unmanaged: true) into GenerateDeploymentV1Beta and keep the require.NotNil assertion — this ensures GenerateDeploymentV1Beta actually checks Database.Unmanaged rather than simply skipping DB injection due to a nil config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apis/mattermost/v1beta1/mattermost_types.go`:
- Around line 361-368: Add kubebuilder CRD validation tags to the
ConnectionStringKey field to enforce allowed secret-key names and length:
annotate the ConnectionStringKey string with
+kubebuilder:validation:Pattern="^[A-Za-z0-9._-]+$" and
+kubebuilder:validation:MaxLength=253 (and keep +optional). Update the struct
field `ConnectionStringKey string `json:"connectionStringKey,omitempty"` in
mattermost_types.go to include these validation markers, then regenerate the
CRD/manifests so the schema includes the pattern and maxLength constraints.
---
Outside diff comments:
In `@config/crd/bases/installation.mattermost.com_mattermosts.yaml`:
- Around line 104-121: Update the Secret description under the "secret" field to
reflect that the key for the connection string is configurable via
connectionStringKey: state that the default key is "DB_CONNECTION_STRING" but if
connectionStringKey is set the operator will use that key (for example "uri"),
and that the operator will also fall back to that key for
MM_SQLSETTINGS_DATASOURCE when no MM_SQLSETTINGS_DATASOURCE entry exists in the
Secret; keep the existing optional keys (MM_SQLSETTINGS_DATASOURCEREPLICAS,
DB_CONNECTION_CHECK_URL) unchanged.
---
Duplicate comments:
In `@apis/mattermost/v1alpha1/zz_generated.openapi.go`:
- Around line 20-338: Generated openapi keys and refs in zz_generated.openapi.go
for package v1alpha1 were rewritten to relative paths
(./apis/mattermost/v1alpha1.*) and double-underscore schema function names
(schema__apis_mattermost_v1alpha1_*), matching the same unwanted change made in
v1beta1; restore the original fully-qualified module-path keys and factory names
by regenerating the OpenAPI files for both v1alpha1 and v1beta1 together from
the correct generator environment or reverting the changes: ensure functions
like schema__apis_mattermost_v1alpha1_ClusterInstallation (and its peers), the
ref(...) targets, and the Dependencies entries use the original
github.com/mattermost/mattermost-operator/apis/mattermost/v1alpha1.*
fully-qualified references instead of ./apis/... so both generated files are
consistent and prevent make generate churn.
---
Nitpick comments:
In `@pkg/mattermost/mattermost_v1beta_test.go`:
- Around line 1125-1127: The test currently passes nil for the DB config so it
can't verify the unmanaged guard; instead construct and pass a non-nil database
config with Unmanaged set true (e.g., a DatabaseConfig/Database struct instance
with Unmanaged: true) into GenerateDeploymentV1Beta and keep the require.NotNil
assertion — this ensures GenerateDeploymentV1Beta actually checks
Database.Unmanaged rather than simply skipping DB injection due to a nil config.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 327aae8a-47b6-4b76-a3d9-f24b8c8db087
📒 Files selected for processing (11)
apis/mattermost/v1alpha1/zz_generated.openapi.goapis/mattermost/v1beta1/db_util.goapis/mattermost/v1beta1/db_util_test.goapis/mattermost/v1beta1/mattermost_types.goapis/mattermost/v1beta1/zz_generated.openapi.goconfig/crd/bases/installation.mattermost.com_mattermosts.yamlcontrollers/mattermost/mattermost/database.gopkg/mattermost/database_external.gopkg/mattermost/database_external_test.gopkg/mattermost/mattermost_v1beta.gopkg/mattermost/mattermost_v1beta_test.go
jalet
force-pushed
the
feat/unmanaged-database
branch
from
f79f0db to
129be17
Compare
Contributor
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
This was referenced May 4, 2026
Adds two independent mechanisms for connecting a Mattermost installation to a pre-existing database secret (e.g. one generated by cloudnative-pg): - spec.database.unmanaged: when true, the operator skips all database configuration. The user is responsible for supplying MM_SQLSETTINGS_* via spec.mattermostEnv, typically via valueFrom.secretKeyRef pointing at an external secret. - spec.database.external.connectionStringKey: overrides the Secret key the operator reads the connection string from (default DB_CONNECTION_STRING). When set and no MM_SQLSETTINGS_DATASOURCE key is present in the Secret, the same key is reused as the MM_SQLSETTINGS_DATASOURCE source, so a single 'uri' entry is enough. Fixes: mattermost#437
libpq-based clients (including pg_isready, which the operator uses for the external-database readiness init container) accept both "postgres://" and the officially-named "postgresql://" URI form. cloudnative-pg and other postgres operators emit the postgresql:// variant, which was previously rejected by validateDBCheckURL. Add postgresql as an allowed scheme for the postgres and unknown db types. GetTypeFromConnectionString already detects both via its "postgres" prefix check, and the connection string itself is passed through unchanged via secretKeyRef, so no further changes are needed. Fixes: mattermost#438
jalet
force-pushed
the
feat/unmanaged-database
branch
from
129be17 to
3b7bee3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two independent improvements to external-database support, aimed at consuming
pre-existing secrets produced by other operators (e.g. cloudnative-pg).
spec.database.unmanaged: true— operator skips all DB config; user suppliesMM_SQLSETTINGS_*viaspec.mattermostEnv.spec.database.external.connectionStringKey— override the Secret key theoperator reads the connection string from (default
DB_CONNECTION_STRING).When set and no
MM_SQLSETTINGS_DATASOURCEkey is present in the Secret, thesame key is reused as the datasource source, so a single
urientry isenough.
postgresql://scheme inDB_CONNECTION_CHECK_URLvalidation —pg_isreadyaccepts both URI forms;GetTypeFromConnectionStringalreadydetected both via its
postgresprefix check.All three are opt-in; default behavior is unchanged.
Ticket Link
Fixes #437
Fixes #438
Release Note