feat: PostgREST image registry and schema preflight (PLAT-499, PLAT-500)#300
Open
moizpgedge wants to merge 3 commits intomainfrom
Open
feat: PostgREST image registry and schema preflight (PLAT-499, PLAT-500)#300moizpgedge wants to merge 3 commits intomainfrom
moizpgedge wants to merge 3 commits intomainfrom
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:
📝 WalkthroughWalkthroughThis pull request adds support for the PostgREST service version 14.5, including service image configuration, updates service version validation patterns to accept two-part version numbers, and introduces a new PostgRESTPreflightResource for pre-deployment schema validation. A helper function refactor extracts shared database connection logic. Changes
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/internal/orchestrator/swarm/service_images_test.go`:
- Around line 109-114: The test case named "postgrest service has versions"
currently only checks the length; change it to assert the actual returned
versions from SupportedServiceVersions("postgrest") rather than just count:
collect the map keys returned by SupportedServiceVersions, sort them (or compare
as an unordered set) and assert they equal the expected set {"latest","v14.5"}
so the test fails if any other two-element combination is returned; update the
assertion in the test function in service_images_test.go accordingly (use the
test helper or testify/reflect-based equality as appropriate).
In `@server/internal/workflows/activities/validate_postgrest_prereqs.go`:
- Around line 134-150: The code currently falls back to db.Instances[0] when
Patroni discovery fails; instead, iterate db.Instances and pick the first
instance whose connection info successfully resolves via
a.DatabaseService.GetInstanceConnectionInfo (and optionally verify
reachability), then assign its InstanceID to primaryInstanceID. Update the
fallback block so that if primaryInstanceID == "" you loop through db.Instances,
call a.DatabaseService.GetInstanceConnectionInfo(ctx, databaseID,
inst.InstanceID), use the first call that returns no error (and/or a lightweight
reachability check) to set primaryInstanceID, rather than unconditionally using
db.Instances[0]; keep references to patroni.NewClient and
database.GetPrimaryInstanceID unchanged.
In `@server/internal/workflows/update_database.go`:
- Around line 106-117: The code currently ignores parseErrs from
ParsePostgRESTServiceConfig and only runs preflight when parseErrs is empty;
change this to fail fast when parseErrs is non-empty: if
ParsePostgRESTServiceConfig returns any parseErrs, return an error (wrap via
handleError) describing the invalid PostgREST config and include parseErrs
details so the workflow stops before planning; keep the existing preflight call
(ExecuteValidatePostgRESTPrereqs with ValidatePostgRESTPrereqsInput using
cfg.DBSchemas and cfg.DBAnonRole) for the valid-path case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc966f26-8fa1-4b2b-acdb-4b3e618ba438
📒 Files selected for processing (5)
server/internal/orchestrator/swarm/service_images.goserver/internal/orchestrator/swarm/service_images_test.goserver/internal/workflows/activities/activities.goserver/internal/workflows/activities/validate_postgrest_prereqs.goserver/internal/workflows/update_database.go
server/internal/workflows/activities/validate_postgrest_prereqs.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Registers PostgREST service images in the Swarm service image registry and adds a preflight workflow activity that validates required Postgres schemas and the configured anonymous role exist before provisioning PostgREST.
Changes:
- Add
ValidatePostgRESTPrereqsworkflow activity to connect to the database and verify configured schemas + anon role exist. - Run the PostgREST preflight checks in
UpdateDatabasebefore planning/provisioning. - Register PostgREST service image versions and extend Swarm service image tests accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/internal/workflows/update_database.go | Runs PostgREST prerequisite validation before the plan/apply phases. |
| server/internal/workflows/activities/validate_postgrest_prereqs.go | New activity that connects to a DB instance and checks schemas + anon role existence. |
| server/internal/workflows/activities/activities.go | Registers the new ValidatePostgRESTPrereqs activity with the worker. |
| server/internal/orchestrator/swarm/service_images.go | Adds PostgREST image entries (latest, v14.5) to the service image registry. |
| server/internal/orchestrator/swarm/service_images_test.go | Adds/updates tests to cover PostgREST image resolution and version listing expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
server/internal/workflows/activities/validate_postgrest_prereqs.go
Outdated
Show resolved
Hide resolved
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 `@server/internal/workflows/activities/validate_postgrest_prereqs.go`:
- Around line 160-163: The current call to
DatabaseService.GetInstanceConnectionInfo(databaseID, primaryInstanceID) returns
an error and bails out immediately; instead, when that call fails, fall back to
the previously-captured reachable connection info (e.g., reachableConnInfo /
reachableInstanceID) for the read-only checks: attempt to assign connInfo =
reachableConnInfo and proceed (optionally log or warn that primary lookup failed
and reachable instance is being used); only return the wrapped error if no
reachableConnInfo is available. Update the logic around connInfo,
primaryInstanceID, and reachableConnInfo in the ValidatePostgrestPrereqs flow to
prefer the primary result but accept the reachable fallback on error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b76c1ad0-1ff4-4562-aa87-401f010c42b8
📒 Files selected for processing (4)
server/internal/orchestrator/swarm/service_images.goserver/internal/orchestrator/swarm/service_images_test.goserver/internal/workflows/activities/validate_postgrest_prereqs.goserver/internal/workflows/update_database.go
server/internal/workflows/activities/validate_postgrest_prereqs.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/internal/api/apiv1/validate.go (1)
260-264: Consider updating the error message to reflect two-part version support.The error message references "semver format (e.g., '1.0.0')" but the validation now also accepts two-part versions like "14.5". Consider updating the message to be more accurate:
- err := errors.New("version must be in semver format (e.g., '1.0.0') or 'latest'") + err := errors.New("version must be in format 'X.Y' or 'X.Y.Z' (e.g., '14.5', '1.0.0') or 'latest'")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/api/apiv1/validate.go` around lines 260 - 264, The validation error message for svc.Version is out of date: update the string created where svc.Version is checked (the block using semverPattern, newValidationError, appendPath, and errs) to accurately state that versions may be semver (e.g., "1.0.0"), two-part versions (e.g., "14.5"), or "latest" so the message matches the accepted formats.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/internal/api/apiv1/validate.go`:
- Around line 260-264: The validation error message for svc.Version is out of
date: update the string created where svc.Version is checked (the block using
semverPattern, newValidationError, appendPath, and errs) to accurately state
that versions may be semver (e.g., "1.0.0"), two-part versions (e.g., "14.5"),
or "latest" so the message matches the accepted formats.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90ed306b-89ae-47e2-b987-2070738d8d56
⛔ Files ignored due to path filters (7)
api/apiv1/gen/control_plane/service.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/types.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/server/types.gois excluded by!**/gen/**api/apiv1/gen/http/openapi.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi.yamlis excluded by!**/gen/**api/apiv1/gen/http/openapi3.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi3.yamlis excluded by!**/gen/**
📒 Files selected for processing (6)
api/apiv1/design/database.goserver/internal/api/apiv1/validate.goserver/internal/api/apiv1/validate_test.goserver/internal/orchestrator/swarm/service_images.goserver/internal/orchestrator/swarm/service_images_test.goserver/internal/workflows/activities/validate_postgrest_prereqs.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/internal/orchestrator/swarm/service_images_test.go
- server/internal/workflows/activities/validate_postgrest_prereqs.go
moizpgedge
force-pushed
the
feat/PLAT-499/PLAT-500/PostgREST-prerequisites/image-wiring
branch
from
99689f0 to
e80ba4c
Compare
Contributor
Author
|
Post Pr for PLAT-501/502/503 PR:-#303 |
jason-lynch
reviewed
Mar 19, 2026
server/internal/workflows/activities/validate_postgrest_prereqs.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/internal/orchestrator/swarm/postgrest_preflight_resource.go (1)
74-81: Logger obtained but never used; error silently discarded.The logger is retrieved but not used anywhere in the
validatemethod. Additionally, the error fromdo.Invokeis discarded with_, which could mask DI configuration issues.Consider either:
- Using the logger to log validation progress/results (e.g., schemas being checked, successful validation)
- Removing the logger invocation if it's not needed
♻️ Option A: Remove unused logger
func (r *PostgRESTPreflightResource) validate(ctx context.Context, rc *resource.Context) error { - logger, _ := do.Invoke[zerolog.Logger](rc.Injector) - - conn, err := connectToPrimaryDB(ctx, rc, r.DatabaseID, r.DatabaseName, logger) + conn, err := connectToPrimaryDB(ctx, rc, r.DatabaseID, r.DatabaseName, zerolog.Nop()) if err != nil { return fmt.Errorf("preflight: failed to connect to database: %w", err) }♻️ Option B: Use logger and handle error
func (r *PostgRESTPreflightResource) validate(ctx context.Context, rc *resource.Context) error { - logger, _ := do.Invoke[zerolog.Logger](rc.Injector) + logger, err := do.Invoke[zerolog.Logger](rc.Injector) + if err != nil { + return fmt.Errorf("failed to get logger: %w", err) + } + logger = logger.With(). + Str("service_id", r.ServiceID). + Str("database_id", r.DatabaseID). + Logger() + logger.Debug().Msg("running PostgREST preflight validation") conn, err := connectToPrimaryDB(ctx, rc, r.DatabaseID, r.DatabaseName, logger)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/postgrest_preflight_resource.go` around lines 74 - 81, The call to do.Invoke[zerolog.Logger](rc.Injector) in PostgRESTPreflightResource.validate either needs its error handled and the logger used, or the unused invocation removed; update the validate function to call do.Invoke and check the returned error (not discard it) and then use the logger to emit at least a start/ok/failure message around connectToPrimaryDB and validation steps (include the database id/name from r.DatabaseID/r.DatabaseName in logs), or simply delete the do.Invoke line if no logger is required—ensure any retained logger variable is referenced so the compiler won’t flag it and that DI errors from do.Invoke are returned instead of ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/internal/orchestrator/swarm/postgrest_preflight_resource.go`:
- Around line 74-81: The call to do.Invoke[zerolog.Logger](rc.Injector) in
PostgRESTPreflightResource.validate either needs its error handled and the
logger used, or the unused invocation removed; update the validate function to
call do.Invoke and check the returned error (not discard it) and then use the
logger to emit at least a start/ok/failure message around connectToPrimaryDB and
validation steps (include the database id/name from r.DatabaseID/r.DatabaseName
in logs), or simply delete the do.Invoke line if no logger is required—ensure
any retained logger variable is referenced so the compiler won’t flag it and
that DI errors from do.Invoke are returned instead of ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11b93e64-2be7-4519-ad38-e99af521a779
📒 Files selected for processing (3)
server/internal/orchestrator/swarm/postgrest_preflight_resource.goserver/internal/orchestrator/swarm/resources.goserver/internal/orchestrator/swarm/service_user_role.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/internal/orchestrator/swarm/resources.go
- Pin postgrest image to ghcr.io/pgedge/postgrest:14.5 - Extend version pattern to accept two-part versions (e.g. 14.5) - Add PostgRESTPreflightResource with PrimaryExecutor to validate that configured schemas and anon role exist before provisioning - Refresh() returns ErrNotFound so the check re-runs on every apply - Extract connectToPrimaryDB as a package-level helper shared between ServiceUserRole and PostgRESTPreflightResource - All config fields optional: db_schemas defaults to "public", db_anon_role to "pgedge_application_read_only" PLAT-499, PLAT-500
moizpgedge
force-pushed
the
feat/PLAT-499/PLAT-500/PostgREST-prerequisites/image-wiring
branch
from
c1b42cd to
f8de470
Compare
PLAT-499, PLAT-500
jason-lynch
reviewed
Mar 25, 2026
Member
jason-lynch
left a comment
jason-lynch
left a comment
There was a problem hiding this comment.
Nice! This is looking pretty close.
server/internal/orchestrator/swarm/postgrest_preflight_resource.go
Outdated
Show resolved
Hide resolved
server/internal/orchestrator/swarm/postgrest_preflight_resource.go
Outdated
Show resolved
Hide resolved
- Remove connectToPrimaryDB helper; use database.GetPrimaryInstance + primary.Connection as the standard connection pattern - Fix permadrift in PostgRESTPreflightResource.Refresh: run validation and only return ErrNotFound when prerequisites fail, avoiding unnecessary updates to dependent resources on every apply cycle - Clean up unused imports (pgx, errors, time, certificates, patroni) PLAT-499, PLAT-500
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
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.
Register PostgREST container images and add schema/role preflight validation
Pins PostgREST to 14.5 from the pgEdge registry (ghcr.io/pgedge/postgrest:14.5). The bare ref lets serviceImageTag prepend the configured ImageRepositoryHost, consistent with how MCP images are registered.
Add schema/role preflight validation before provisioning PostgREST. Before the plan step, the workflow connects to the primary Postgres instance and verifies that all configured schemas and the anonymous role exist. Missing objects produce a clear error, e.g.:
PLAT-499, PLAT-500
Summary
Registers the PostgREST service image (ghcr.io/pgedge/postgrest:14.5) and adds a preflight check that validates required schemas and the anonymous role exist in Postgres before PostgREST provisioning begins. Also extends the service version pattern to accept two-part versions (e.g., 14.5) in both the Goa design and server-side validation.
All PostgREST config fields are optional with sensible defaults — db_schemas defaults to "public" and db_anon_role defaults to "pgedge_application_read_only". A minimal PostgREST deployment requires only
"config": {}.Optional:-
Changes
Testing
Checklist
Notes for Reviewers
Preflight connects read-only; falls back to the first reachable instance if primary lookup via Patroni fails (stale data edge case)
"config": {} is valid and sufficient for a standard PostgREST deployment against the public schema