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:
📝 WalkthroughWalkthroughAdds PostgREST support: extensive e2e tests, refactors service instance naming (removes serviceType and enforces 63-char limit), introduces PostgREST preflight database dependency, selects RW service user role for PostgREST, targets primary DB connections, and adds documentation and examples. Changes
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (3)
e2e/postgrest_test.go (2)
36-45: Minor: node naming logic may produce unexpected results for 2-node clusters.When
len(nodeHosts) == 2, the slice indexing at line 43 will work (["n1", "n2", "n3"][1]= "n2"), but the first node at index 0 will haveName: "n1"from line 39 (set before the conditional). This is correct but the logic flow is slightly confusing. Consider restructuring for clarity if this helper is used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/postgrest_test.go` around lines 36 - 45, The node naming logic in the nodes creation loop mixes an initial default Name assignment with a conditional override, which is confusing for 2-node clusters; update the loop that fills nodes (the nodes slice of *controlplane.DatabaseNodeSpec) to determine the name list first (e.g., names := []string{"n1","n2","n3"} truncated to len(nodeHosts)) and then assign Name from that list for each index i, instead of setting Name to "n1" first and conditionally overriding it, so DatabaseNodeSpec.Name is consistently assigned in one place.
489-512: Loop only checks single node despite comment implying all nodes.The comment at line 488 says "confirm service roles exist on all nodes" but the loop iterates only over
[]string{"n1"}. If checking multiple nodes was intended, update the loop. If single-node check is intentional, consider updating the comment to match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/postgrest_test.go` around lines 489 - 512, The test's loop over nodes uses a hardcoded slice []string{"n1"} but the preceding comment says "confirm service roles exist on all nodes"; update the slice used in the for loop (the []string literal referenced by nodeName) to include all intended node names (e.g., "n1","n2", etc.) so the check runs per-node, or alternatively change the comment to accurately state that the check is only for node "n1" if a single-node check was intended; the change should be made where the for _, nodeName := range []string{"n1"} { loop is declared.server/internal/orchestrator/swarm/orchestrator_test.go (1)
7-62: Well-structured property-based tests for the new naming scheme.The test correctly validates the core contract properties:
- Docker Swarm's 63-character limit
- Determinism (same inputs → same output)
- Coverage for PostgREST service type and long service type truncation
One optional enhancement: consider validating that the generated name conforms to Docker Swarm's character constraints (lowercase alphanumeric and hyphens, no leading/trailing hyphens). This would catch any future implementation changes that accidentally produce invalid service names.
🧪 Optional: Add character validation helper
+import ( + "regexp" + "testing" +) + +// dockerSwarmNamePattern matches valid Docker Swarm service names. +var dockerSwarmNamePattern = regexp.MustCompile(`^[a-z0-9][a-z0-9-]*[a-z0-9]$|^[a-z0-9]$`) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got := ServiceInstanceName(tt.serviceType, tt.databaseID, tt.serviceID, tt.hostID) // Must be within Docker Swarm's 63-char limit. if len(got) > 63 { t.Errorf("ServiceInstanceName() = %q (len %d), must be <= 63 chars", got, len(got)) } + // Must be a valid Docker Swarm service name. + if !dockerSwarmNamePattern.MatchString(got) { + t.Errorf("ServiceInstanceName() = %q is not a valid Docker Swarm service name", got) + } + // Must be deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/orchestrator_test.go` around lines 7 - 62, Add a character-validation assertion to the TestServiceInstanceName tests: after computing got (from ServiceInstanceName(serviceType, databaseID, serviceID, hostID)), verify the name is all lowercase and matches Docker Swarm's allowed pattern (only lowercase letters, digits and hyphens, no leading or trailing hyphen) — e.g. validate against the regex for valid service names and assert the match; reference the ServiceInstanceName function when locating where to add this check.
🤖 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/orchestrator.go`:
- Around line 159-174: The ServiceInstanceName change will orphan existing MCP
services; update the lookup/migration so existing deployments are found or
cleaned up: add a legacy-name fallback and/or one-time migration when resolving
services by name in ServiceInstance.Sync (and where generateMCPInstanceResources
calls ServiceInstanceName for "mcp"/"postgrest"): implement a fallback that
computes the old name format (the previous
`{serviceType}-{databaseID}-{serviceID}-{sha1(hostID)[:8]}` scheme) and try
ServiceInspectByLabels/ServiceInspectByName using that legacy name before
declaring a missing service, or add a migration/cleanup routine that enumerates
services (using labels like pgedge.service) and renames/removes or re-labels
them to the new ServiceInstanceName format.
In `@server/internal/orchestrator/swarm/service_instance_spec.go`:
- Around line 85-96: The PostgREST branch sets the local variable role to an
invalid value ("postgrest_authenticator"); update the PostgREST case in the
block that computes mode and role (where ServiceUserRoleRO/ServiceUserRoleRW and
s.ServiceSpec.ServiceType are used) so that role is "pgedge_application" for the
RW path instead of "postgrest_authenticator", ensuring ServiceUser.Role matches
the allowed values used by ServiceUserRole.createUserRole() and
ServiceUserRoleIdentifier.
---
Nitpick comments:
In `@e2e/postgrest_test.go`:
- Around line 36-45: The node naming logic in the nodes creation loop mixes an
initial default Name assignment with a conditional override, which is confusing
for 2-node clusters; update the loop that fills nodes (the nodes slice of
*controlplane.DatabaseNodeSpec) to determine the name list first (e.g., names :=
[]string{"n1","n2","n3"} truncated to len(nodeHosts)) and then assign Name from
that list for each index i, instead of setting Name to "n1" first and
conditionally overriding it, so DatabaseNodeSpec.Name is consistently assigned
in one place.
- Around line 489-512: The test's loop over nodes uses a hardcoded slice
[]string{"n1"} but the preceding comment says "confirm service roles exist on
all nodes"; update the slice used in the for loop (the []string literal
referenced by nodeName) to include all intended node names (e.g., "n1","n2",
etc.) so the check runs per-node, or alternatively change the comment to
accurately state that the check is only for node "n1" if a single-node check was
intended; the change should be made where the for _, nodeName := range
[]string{"n1"} { loop is declared.
In `@server/internal/orchestrator/swarm/orchestrator_test.go`:
- Around line 7-62: Add a character-validation assertion to the
TestServiceInstanceName tests: after computing got (from
ServiceInstanceName(serviceType, databaseID, serviceID, hostID)), verify the
name is all lowercase and matches Docker Swarm's allowed pattern (only lowercase
letters, digits and hyphens, no leading or trailing hyphen) — e.g. validate
against the regex for valid service names and assert the match; reference the
ServiceInstanceName function when locating where to add this check.
🪄 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: 21e8d5c6-3612-49e7-8d13-b8d90a236122
📒 Files selected for processing (9)
docs/Supported-services/Postgrest-docs.mde2e/postgrest_test.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/orchestrator_test.goserver/internal/orchestrator/swarm/postgrest_preflight_resource.goserver/internal/orchestrator/swarm/service_images.goserver/internal/orchestrator/swarm/service_instance_spec.goserver/internal/orchestrator/swarm/service_spec.goserver/internal/workflows/plan_update.go
| // databaseID and serviceID are often UUIDs or long strings, all four components | ||
| // are hashed together into a single 16-char base-36 suffix. The serviceType | ||
| // prefix is kept for readability and is truncated to 10 chars to fit comfortably. | ||
| func ServiceInstanceName(serviceType, databaseID, serviceID, hostID string) string { |
There was a problem hiding this comment.
What's the reason for this change? This might break existing services, and it deviates from the database instance service name implementation.
There was a problem hiding this comment.
This handles the case where databaseID and serviceID are UUIDs Since services are discovered by label not name, the rename just triggers a brief restart on next reconciliation rather than orphaning anything. Happy to revert if you prefer keeping it closer to the existing pattern.
There was a problem hiding this comment.
Yes, please revert this.
Host IDs and Database IDs need to be globally unique, whereas service IDs only need to be unique within a database, so it's much less likely that someone will use a UUID for a service ID. The previous implementation was human-readable and matched the same scheme as database instances, which is very handy when you're trying to troubleshoot.
There was a problem hiding this comment.
If you're looking to shorten these IDs: do we need to include serviceType?
The reason that the Postgres service includes the word postgres is that it allows for other database services that do not have a unique identifier. We don't have the same problem with services because they already have a unique identifier.
There was a problem hiding this comment.
i have reverted this If you're looking to shorten these IDs: do we need to include serviceType?
the answer is :No
moizpgedge
force-pushed
the
feat/PLAT-504-505-506-507/Wire-PostgREST-into-the-orchestrator
branch
2 times, most recently
from
a661851 to
d7ede13
Compare
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/orchestrator/swarm/orchestrator.go`:
- Around line 170-173: ServiceInstanceName can produce names longer than Docker
Swarm's 63-char limit; change it to enforce a max total length of 63 by
reserving 10 chars for two separators and the 8-char hash (allowedSum = 53) and
truncating databaseID and serviceID so len(databaseID)+len(serviceID) <=
allowedSum (e.g., if sum > allowedSum, truncate databaseID to at most
allowedSum/2 and then truncate serviceID to allowedSum - len(databaseID) so the
final fmt.Sprintf("%s-%s-%s", databaseID, serviceID, base36[:8]) is <=63), and
add a test case in orchestrator_test.go with maximum-length identifiers (63-char
databaseID and 63-char serviceID) to assert the produced name length is <=63.
🪄 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: 6444cf09-7dfd-4d46-9c05-a4803c93072e
📒 Files selected for processing (9)
docs/Supported-services/Postgrest-docs.mde2e/postgrest_test.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/orchestrator_test.goserver/internal/orchestrator/swarm/postgrest_preflight_resource.goserver/internal/orchestrator/swarm/service_images.goserver/internal/orchestrator/swarm/service_instance_spec.goserver/internal/orchestrator/swarm/service_spec.goserver/internal/workflows/plan_update.go
✅ Files skipped from review due to trivial changes (5)
- server/internal/orchestrator/swarm/service_images.go
- server/internal/orchestrator/swarm/postgrest_preflight_resource.go
- server/internal/orchestrator/swarm/service_spec.go
- docs/Supported-services/Postgrest-docs.md
- e2e/postgrest_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/internal/orchestrator/swarm/service_instance_spec.go
- server/internal/orchestrator/swarm/orchestrator_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/services/postgrest.md (1)
157-159: Normalize code block style to satisfy MD046.markdownlint flags these blocks as fenced where indented style is expected (Lines 157, 166, 204, 232).
Also applies to: 166-168, 204-206, 232-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/services/postgrest.md` around lines 157 - 159, Replace the fenced code blocks shown (for example the block containing the text "http://{host}:{port}/{table_or_view}") with indented code block style to satisfy MD046; locate each occurrence of those fenced triple-backtick blocks (the ones at the examples matching that URL snippet and the other fenced blocks flagged) and convert them to standard indented code blocks (prefix each line with four spaces) so markdownlint no longer reports fenced blocks where indented style is expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/services/postgrest.md`:
- Around line 201-206: The GRANT example uses the wrong target role; update the
SQL to grant the role named in the `role` JWT claim to the PostgREST
authenticator role (postgrest_authenticator) instead of
pgedge_application_read_only so JWT role switching works; locate the example
around the `GRANT app TO pgedge_application_read_only;` snippet and change it to
grant the claimed role to postgrest_authenticator (mentioning the `role` claim
and `postgrest_authenticator`) so the doc shows the correct grant for PostgREST
auth switching.
In `@server/internal/orchestrator/swarm/orchestrator.go`:
- Around line 171-191: The current name-generation uses
sha256.Sum256([]byte(hostID)) (variable base36) but truncates
databaseID/serviceID later, which can cause deterministic collisions; change the
hash input to include hostID, databaseID and serviceID (e.g., concatenate with a
separator) before computing sha256 and converting to base36 so the final
base36[:8] reflects all three identifiers; keep the existing truncation logic
for databaseID/serviceID and the final fmt.Sprintf("%s-%s-%s", databaseID,
serviceID, base36[:8]) format.
---
Nitpick comments:
In `@docs/services/postgrest.md`:
- Around line 157-159: Replace the fenced code blocks shown (for example the
block containing the text "http://{host}:{port}/{table_or_view}") with indented
code block style to satisfy MD046; locate each occurrence of those fenced
triple-backtick blocks (the ones at the examples matching that URL snippet and
the other fenced blocks flagged) and convert them to standard indented code
blocks (prefix each line with four spaces) so markdownlint no longer reports
fenced blocks where indented style is expected.
🪄 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: 7efea2f0-27a1-450a-8d0e-161012e2aca4
📒 Files selected for processing (5)
docs/services/index.mddocs/services/managing.mddocs/services/postgrest.mdserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/orchestrator_test.go
✅ Files skipped from review due to trivial changes (2)
- docs/services/managing.md
- docs/services/index.md
🚧 Files skipped from review as they are similar to previous changes (1)
- server/internal/orchestrator/swarm/orchestrator_test.go
| The `role` claim must name a PostgreSQL role granted to the PostgREST | ||
| authenticator. Grant the role before sending authenticated requests: | ||
|
|
||
| ```sql | ||
| GRANT app TO pgedge_application_read_only; | ||
| ``` |
There was a problem hiding this comment.
Role grant example targets the wrong PostgREST role.
Line 205 grants to pgedge_application_read_only, but PostgREST auth switching is wired through the authenticator role (postgrest_authenticator). Following this doc as-is can make JWT role switching fail.
Suggested doc fix
-GRANT app TO pgedge_application_read_only;
+GRANT app TO postgrest_authenticator;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The `role` claim must name a PostgreSQL role granted to the PostgREST | |
| authenticator. Grant the role before sending authenticated requests: | |
| ```sql | |
| GRANT app TO pgedge_application_read_only; | |
| ``` | |
| The `role` claim must name a PostgreSQL role granted to the PostgREST | |
| authenticator. Grant the role before sending authenticated requests: | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 204-204: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/services/postgrest.md` around lines 201 - 206, The GRANT example uses
the wrong target role; update the SQL to grant the role named in the `role` JWT
claim to the PostgREST authenticator role (postgrest_authenticator) instead of
pgedge_application_read_only so JWT role switching works; locate the example
around the `GRANT app TO pgedge_application_read_only;` snippet and change it to
grant the claimed role to postgrest_authenticator (mentioning the `role` claim
and `postgrest_authenticator`) so the doc shows the correct grant for PostgREST
auth switching.
| // different databases or services on the same host. serviceType is omitted because | ||
| // serviceID is already unique within a database. | ||
| func ServiceInstanceName(databaseID, serviceID, hostID string) string { | ||
| hash := sha256.Sum256([]byte(databaseID + ":" + serviceID + ":" + hostID)) |
There was a problem hiding this comment.
Could you please change this back to:
hash := sha1.Sum([]byte(hostID))That will make this hash match the scheme we use for instances. Sometimes security tools alert about using sha1 because they want to make sure you're not using it for cryptographic purposes. This is not a cryptographic use case, so it's fine to ignore the alert.
There was a problem hiding this comment.
My main issue with changing this scheme is that it makes services and database instances different. I understand you're addressing the service name length issue. Could you please open a dedicated ticket for it and not address it in this PR? That way, we can ensure service names remain consistent across both services and database instances.
There was a problem hiding this comment.
Ok i will open a new ticket for this issue.
moizpgedge
changed the title
moizpgedge
force-pushed
the
feat/PLAT-504-505-506-507/Wire-PostgREST-into-the-orchestrator
branch
from
febafb8 to
87e02af
Compare
…506, PLAT-507)
- Route target_session_attrs=read-write for postgrest in plan_update
- PostgREST container env contains only PGRST_* vars; connection details live in postgrest.conf db-uri
- Register "latest" image version for postgrest
- populateCredentials reads from _rw role for postgrest (authenticator role)
- Guard DataDirID usage so non-data-dir services don't break
- Fix ServiceInstanceName to hash variable inputs into a 16-char base36 suffix, staying well under Docker Swarm's 63-char limit
- Add PostgRESTPreflightResource dependency on PostgresDatabaseResourceIdentifier
- Add docs/Supported-services/Postgrest-docs.md
- Add e2e/postgrest_test.go covering provisioning, lifecycle, JWT, failover, and role verification
fix: replace SHA1 with SHA256 in ServiceInstanceName; fix preflight E2E tests
- Use crypto/sha256 instead of crypto/sha1 to satisfy static analysis
- Preflight E2E tests now create the DB first, then update with the
bad config — asserting the update task fails rather than expecting
NewDatabaseFixture itself to fail
fix:small docs fix
refactor: simplify ServiceInstanceName, resolve rebase conflict
Drop serviceType from ServiceInstanceName — serviceID is already
unique within a database, so the type prefix adds no value. The
output now follows the same {databaseID}-{serviceID}-{8charHash}
scheme as InstanceIDFor, hashing only hostID for readability.
Also resolves rebase conflict in plan_update.go: keeps both the
rag and postgrest cases in resolveTargetSessionAttrs.
PLAT-504, PLAT-505, PLAT-506, PLAT-507
chore: update NOTICE.txt
Fix stale etcd license URL caught by licenses-ci.
PLAT-504
chore: fix NOTICE.txt etcd license URL
fix: enforce 63-char limit in ServiceInstanceName
databaseID and serviceID can each be up to 63 chars. The previous
implementation could produce names up to 136 chars, exceeding Docker
Swarm's limit. Now caps the combined ID budget to 53 chars (giving
databaseID up to 26, serviceID the rest), keeping the total ≤ 63.
Adds three test cases covering max-length inputs.
docs: add PostgREST service documentation
Add postgrest.md covering configuration reference, JWT auth,
preflight checks, multi-host failover, and schema cache reload.
Remove the old draft Postgrest-docs.md. Update index.md to link
PostgREST as a live service type and managing.md to include a
PostgREST provisioning example.
PLAT-504, PLAT-505, PLAT-506, PLAT-507
fix: hash all inputs in ServiceInstanceName; fix JWT doc
Include databaseID, serviceID, and hostID in the hash so truncated
prefixes never collide for different services on the same host.
Fix the JWT role grant example in postgrest.md — the authenticator
username is auto-managed, so remove the wrong SQL and direct users
to the database status response for the correct username.
PLAT-504, PLAT-507
Match the hashing scheme used by InstanceIDFor, as requested in code review. SHA1 is not used here for cryptographic purposes. PLAT-504
moizpgedge
force-pushed
the
feat/PLAT-504-505-506-507/Wire-PostgREST-into-the-orchestrator
branch
from
87e02af to
3b95c2d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/services/postgrest.md (1)
201-204: Consider adding an explicit GRANT example.The text explains that roles must be granted to the authenticator user and where to find the username, but doesn't show the actual SQL. Adding a brief example would help users:
-- After finding the authenticator username (e.g., svc_postgrest-api_rw) GRANT app TO svc_postgrest_api_rw;This is optional since the concept is explained, but an example would reduce friction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/services/postgrest.md` around lines 201 - 204, Add a short, explicit SQL GRANT example to the PostgREST docs near the paragraph about the `role` claim and `service_instances` field: show how to grant an application role to the authenticator user (e.g., `GRANT app TO svc_postgrest_api_rw;`) and include a brief comment indicating where to substitute the actual authenticator username discovered from `service_instances`; keep it one or two lines and place it immediately after the sentence that instructs users to grant roles to that user.e2e/postgrest_test.go (2)
488-514: Loop only checks one node despite comment claiming "all nodes".The comment at line 488 says "Connect to Postgres and confirm service roles exist on all nodes" but the loop at line 489 only iterates over
[]string{"n1"}. For a 3-node database spec, this doesn't verify nodes n2/n3.If the intent is to verify only the primary (which may be on any node), the comment should be updated. If roles should be verified on all nodes, expand the loop.
Suggested fix if checking only primary is intentional
- // Connect to Postgres and confirm service roles exist on all nodes. - for _, nodeName := range []string{"n1"} { + // Connect to the primary and confirm service roles exist. + { + nodeName := "n1"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/postgrest_test.go` around lines 488 - 514, The loop and comment disagree: the for loop only iterates over []string{"n1"} but the comment for ConnectToInstance/ConnectionOptions (using Matcher, WithNode, WithRole) claims "all nodes"; either expand the slice to include all cluster nodes (e.g., "n1","n2","n3") so the block that calls db.ConnectToInstance and checks pg_roles runs on every node, or update the comment to state that the test only verifies the primary (since the fallback uses WithRole("primary")). Modify the loop slice or the comment accordingly and keep the existing fallback connection logic intact.
611-616: Consider polling instead of fixed sleep.The 15-second sleep assumes a fixed reconnection time. If PostgREST reconnects faster, the test wastes time. If it takes longer, the test could flake. Consider polling the service state or health endpoint with a timeout instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/postgrest_test.go` around lines 611 - 616, Replace the fixed 15s sleep with a polling loop that repeatedly checks the PostgREST status until success or timeout: poll db.ServiceInstances[0].State (or the PostgREST health endpoint if available) using a ticker and a time.After deadline, break early when the state is no longer "failed", and only assert failure if the timeout elapses; update the code around the time.Sleep and the assertion that reads db.ServiceInstances[0] / si.State to use this timeout+polling pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/services/postgrest.md`:
- Around line 230-235: The admin port example is inconsistent: update the curl
example URL used in the PostgREST docs so it targets the admin port `{port} + 1`
for a main port of `3100`; specifically replace the example `curl -X POST
http://host-1:3001/notify-reload` with `curl -X POST
http://host-1:3101/notify-reload` so it matches the explanatory sentence about
the admin port.
---
Nitpick comments:
In `@docs/services/postgrest.md`:
- Around line 201-204: Add a short, explicit SQL GRANT example to the PostgREST
docs near the paragraph about the `role` claim and `service_instances` field:
show how to grant an application role to the authenticator user (e.g., `GRANT
app TO svc_postgrest_api_rw;`) and include a brief comment indicating where to
substitute the actual authenticator username discovered from
`service_instances`; keep it one or two lines and place it immediately after the
sentence that instructs users to grant roles to that user.
In `@e2e/postgrest_test.go`:
- Around line 488-514: The loop and comment disagree: the for loop only iterates
over []string{"n1"} but the comment for ConnectToInstance/ConnectionOptions
(using Matcher, WithNode, WithRole) claims "all nodes"; either expand the slice
to include all cluster nodes (e.g., "n1","n2","n3") so the block that calls
db.ConnectToInstance and checks pg_roles runs on every node, or update the
comment to state that the test only verifies the primary (since the fallback
uses WithRole("primary")). Modify the loop slice or the comment accordingly and
keep the existing fallback connection logic intact.
- Around line 611-616: Replace the fixed 15s sleep with a polling loop that
repeatedly checks the PostgREST status until success or timeout: poll
db.ServiceInstances[0].State (or the PostgREST health endpoint if available)
using a ticker and a time.After deadline, break early when the state is no
longer "failed", and only assert failure if the timeout elapses; update the code
around the time.Sleep and the assertion that reads db.ServiceInstances[0] /
si.State to use this timeout+polling pattern.
🪄 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: 13a2fd5d-e628-4746-b56b-54d47759bf61
📒 Files selected for processing (11)
docs/services/index.mddocs/services/managing.mddocs/services/postgrest.mde2e/postgrest_test.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/orchestrator_test.goserver/internal/orchestrator/swarm/postgrest_preflight_resource.goserver/internal/orchestrator/swarm/service_images.goserver/internal/orchestrator/swarm/service_instance_spec.goserver/internal/orchestrator/swarm/service_spec.goserver/internal/workflows/plan_update.go
✅ Files skipped from review due to trivial changes (2)
- docs/services/index.md
- server/internal/orchestrator/swarm/service_spec.go
🚧 Files skipped from review as they are similar to previous changes (6)
- server/internal/orchestrator/swarm/service_images.go
- server/internal/orchestrator/swarm/postgrest_preflight_resource.go
- server/internal/workflows/plan_update.go
- docs/services/managing.md
- server/internal/orchestrator/swarm/service_instance_spec.go
- server/internal/orchestrator/swarm/orchestrator_test.go
2 participants
Summary
Wires PostgREST into the orchestrator as a fully functional service type: connection routing, container spec, credential role selection, service name collision fix, preflight dependency, docs, and E2E tests.
Changes
plan_update.go: routetarget_session_attrs=read-writefor postgrest so libpq always connects to the primaryservice_spec.go: PostgREST container env is exactly 3PGRST_*vars — connection details live inpostgrest.confdb-uri onlyservice_images.go: register"latest"image version for postgrestservice_instance_spec.go:populateCredentialsreads from_rwrole for postgrest (authenticator); guardDataDirIDso services without a data dir don't breakorchestrator.go: fixServiceInstanceName— hash all variable inputs into a 16-char base36 suffix to stay under Docker Swarm's 63-char limitpostgrest_preflight_resource.go: addPostgresDatabaseResourceIdentifieras a dependency so preflight runs after the DB is readydocs/Supported-services/Postgrest-docs.md: operator runbook — DBA grants, config reference, deploy/remove commandse2e/postgrest_test.go: 11 E2E tests covering provisioning, JWT auth, preflight failures, role verification, config updates, multi-host, and failoverTesting
Checklist
Notes for Reviewers