Expand query and database capabilities#32
Open
moshloop wants to merge 12 commits into
Open
Conversation
Prevent migration DDL and security reconciliation from waiting indefinitely on locks. Retry transient lock contention so live readers no longer cause migration runs to hang or fail unnecessarily.
…onnections Adds first-class OpenTelemetry connections backed by validated nested OpenSearch connections. Supports Jaeger and flat trace querying, normalized result fields, filtering, discovery, testing, and schema-driven connection selection.
Enable reusable profile composition with cycle detection, ordered row aliases, ignored fields, and derived columns. Add OpenTelemetry provider/schema support and migrate legacy trace profiles while preserving connections and imports. Pass validated parameters to providers and persist query application properties.
Prompt for a missing OpenTelemetry connection, persist the selected mapping, and retry profile execution. Add an inline connection form when no compatible connection exists.
Adopt Captain’s shared chat runtime and tool metadata contract for the query assistant Preserve query-specific filtering while applying server-managed tool permissions Allow read operations automatically and prompt for mutating or unknown operations
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
WalkthroughThe pull request adds OpenTelemetry connection and query support, recursive profile resolution and legacy migration, profile-to-connection mapping UI, Captain chat integration, migration lock handling, database test infrastructure, embedded PostgreSQL diagnostics, profile-building UI, and related schemas and tests. ChangesOpenTelemetry query and profile execution
Profile resolution and legacy migration
Query chat and application wiring
Migration lock safety
Database test infrastructure and CI
Embedded PostgreSQL diagnostics
Profile-building UI
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (3)
cmd/query/connections/service_test.go (1)
45-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the nested connection type check.
The negative case only tests self-reference, which fails before lookup. Add a PostgreSQL or HTTP target assertion to cover the OpenSearch-only invariant in
validateNestedConnection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/query/connections/service_test.go` around lines 45 - 62, The test TestValidateOpenTelemetryRequiresNestedOpenSearchConnection currently covers only self-reference; add a negative case where the OpenTelemetry candidate’s nested connection points to a persisted PostgreSQL or HTTP connection. Assert validateNestedConnection returns an error for that non-OpenSearch target, while preserving the existing valid OpenSearch and self-reference assertions.migrate/migrate_test.go (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming the test case to accurately reflect the data.
The test case is named "url without query...", but the connection string contains the query parameter
?sslmode=disable. Updating the name to reflect that it specifically lacks theoptionsquery will make it more accurate.💡 Proposed renaming
- name: "url without query gains a bounded lock_timeout", + name: "url without options query gains a bounded lock_timeout",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@migrate/migrate_test.go` at line 33, Rename the test case from “url without query gains a bounded lock_timeout” to wording that accurately states the URL lacks the options query, while preserving the test’s existing data and behavior.migrate/scripts.go (1)
319-319: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant context from the exhaustion error.
Consider removing
%s:(anddesc) from this returned exhaustion error.The outer callers (e.g.,
reconcileSecurity,dropDependentView, andapplyTransactionalScript) already wrap the returned error with their own context string. Removingdeschere prevents redundant double-wrapping in the final error string (e.g.,execute SQL migration ...: execute SQL migration ...: gave up ...), while thelogger.Debugfinside the loop continues to correctly log the specific attempt context.♻️ Proposed fix
- return fmt.Errorf("%s: gave up after %d attempts under lock contention: %w", desc, migrationMaxAttempts, lastErr) + return fmt.Errorf("gave up after %d attempts under lock contention: %w", migrationMaxAttempts, lastErr)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@migrate/scripts.go` at line 319, Update the exhaustion error return in the retry logic to remove the redundant desc prefix and return only the attempts-exhausted message while preserving lastErr wrapping. Keep desc available for the existing logger.Debugf attempt context, and leave the outer callers’ contextual wrapping unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/query/connections/service.go`:
- Around line 235-240: Update the nested connection resolution around
findConnection to use the shared connection URL hydrator/parser and preserve the
full connection reference, rather than trimming it to the final path component.
Ensure validation resolves connection://team/OS and other URL forms with the
same semantics used at runtime.
In `@cmd/query/profiles/execution.go`:
- Around line 173-207: Update mapConnection to validate
resolved.Profile.Provider.Type before calling h.store.Get or modifying
owner.Provider.Connection. Reject profiles whose provider type is not
OpenTelemetry with an appropriate HTTP bad-request response, while preserving
the existing connection-type validation and save flow for OpenTelemetry
profiles.
In `@cmd/query/profiles/resolver.go`:
- Around line 39-42: Update the merge logic around mergeProfile so
result.ConnectionProfile is replaced only when imported.Profile contributes an
effective connection change, not merely when its provider type is non-empty.
Preserve the existing connection ownership when a later import has the same
provider type but no connection, ensuring mapping uses the profile that supplied
the resolved connection.
- Around line 103-108: Update the overlay merge logic in the resolver to enforce
mutual exclusivity: when overlay.Trace is set, clear the inherited merged.Top
before assigning Trace, and when overlay.Top is set, clear merged.Trace before
assigning Top. Preserve the existing overlay precedence while ensuring the
resolved profile contains only the selected kind.
In `@cmd/query/www/src/profileConnectionMapping.tsx`:
- Around line 153-165: Update the dialog rendering near profileConnectionOptions
and the existing empty-state branch to handle connections.isError before
treating an empty options result as “no connection exists.” Show the backend
error state with a retry action wired to the query’s refetch method, while
preserving the current empty-state behavior for successful empty results.
- Around line 73-77: The waitForMapping callback currently overwrites an
unresolved pending request, leaving the earlier executeCommand promise
unsettled. Update the pending-request handling around waitForMapping and its
associated mapping-resolution flow to preserve existing requests by queueing
them, or explicitly reject the previous request before storing the new one;
ensure every concurrent request resolves or rejects.
In `@query/cel.go`:
- Around line 15-50: Update the sampling flow in query/sample.go to call
applyRowTransforms for provider rows before inferring columns and returning
preview results. Reuse the existing transformation behavior so sampled results
include aliases and computed columns while honoring ignored fields, and
propagate any transformation errors.
- Around line 27-36: Update the alias filtering flow around aliasRoots and the
profile.Ignore loop so ignored roots are still deleted from the original row
even when they have aliases. Preserve the projected alias outputs separately,
apply delete(row, ignored) and deleteRowPath(row, ignored), then restore the
saved alias paths afterward so request.xml remains available without retaining
unrelated request.* fields.
- Around line 61-65: Update the environment-building logic around the reserved
"row" and "span" bindings so matching provider fields cannot overwrite those
initial whole-row values. Continue exposing other CEL-identifier-compatible
fields directly, while preserving row.traceId and legacy span.* projections.
In `@query/providers/opentelemetry_parser.go`:
- Around line 177-197: The traceDurationMillis function must stop inferring
nanosecond units from the number > 1_000_000 threshold. Use the format or an
explicit schema/options-provided time unit to select the conversion path,
ensuring small nanosecond durations such as 500,000 are converted correctly
while preserving the existing Jaeger behavior.
- Around line 198-215: Update normalizeTraceTimestamp to recognize microsecond
Unix timestamps explicitly before the nanosecond branch, converting them with
the appropriate time.Unix-based microsecond handling. Preserve the existing
seconds, milliseconds, nanoseconds, RFC3339Nano, and raw-value fallback
behavior.
In `@query/providers/opentelemetry.go`:
- Around line 93-96: Update the row-normalization loops around openTelemetryRow
to consume each hit’s fields response before conversion, including the
corresponding path near the other referenced location. Merge field-only values
from hit.Fields into the document passed to openTelemetryRow, or ensure
selectFields requests them through _source.includes, while preserving existing
source values and selected-field behavior.
---
Nitpick comments:
In `@cmd/query/connections/service_test.go`:
- Around line 45-62: The test
TestValidateOpenTelemetryRequiresNestedOpenSearchConnection currently covers
only self-reference; add a negative case where the OpenTelemetry candidate’s
nested connection points to a persisted PostgreSQL or HTTP connection. Assert
validateNestedConnection returns an error for that non-OpenSearch target, while
preserving the existing valid OpenSearch and self-reference assertions.
In `@migrate/migrate_test.go`:
- Line 33: Rename the test case from “url without query gains a bounded
lock_timeout” to wording that accurately states the URL lacks the options query,
while preserving the test’s existing data and behavior.
In `@migrate/scripts.go`:
- Line 319: Update the exhaustion error return in the retry logic to remove the
redundant desc prefix and return only the attempts-exhausted message while
preserving lastErr wrapping. Keep desc available for the existing logger.Debugf
attempt context, and leave the outer callers’ contextual wrapping unchanged.
🪄 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: b665508b-7f44-4fdc-bfac-b11024a5ab64
⛔ Files ignored due to path filters (1)
cmd/query/go.sumis excluded by!**/*.sum
📒 Files selected for processing (80)
cmd/query/connections/actions.gocmd/query/connections/browser.gocmd/query/connections/info.gocmd/query/connections/service.gocmd/query/connections/service_test.gocmd/query/go.modcmd/query/internal/app/chat.gocmd/query/internal/app/chat_test.gocmd/query/internal/app/migrations/query.hclcmd/query/internal/app/migrations_test.gocmd/query/internal/app/schema_handler.gocmd/query/internal/app/server.gocmd/query/profiles/execution.gocmd/query/profiles/execution_test.gocmd/query/profiles/icons.gocmd/query/profiles/legacy_migration.gocmd/query/profiles/legacy_migration_test.gocmd/query/profiles/legacy_provider.gocmd/query/profiles/openapi.gocmd/query/profiles/resolver.gocmd/query/profiles/resolver_test.gocmd/query/profiles/service.gocmd/query/profiles/store.gocmd/query/profiles/store_test.gocmd/query/profiles/suite_test.gocmd/query/sessions/service.gocmd/query/sessions/top.gocmd/query/sessions/trace.gocmd/query/www/src/App.tsxcmd/query/www/src/chatWidget.tsxcmd/query/www/src/connectionSchemaMonaco.test.tscmd/query/www/src/profileConnectionMapping.test.tscmd/query/www/src/profileConnectionMapping.tsxconnection/opentelemetry.goconnection/opentelemetry_test.godb/embedded.godb/embedded_test.gogo.modmigrate/README.mdmigrate/migrate.gomigrate/migrate_test.gomigrate/scripts.gomigrate/security.gomigrate/view_deps.gomodels/connections.goquery/cel.goquery/engine.goquery/engine_test.goquery/iterator.goquery/profile.goquery/provider.goquery/providers/new_providers_test.goquery/providers/opensearch.goquery/providers/opentelemetry.goquery/providers/opentelemetry_parser.goquery/sample.goquery/schema/connection.goquery/schema/connection_providers.goquery/schema/connection_types.goquery/schema/profile.goquery/schema/profile_components.goquery/schema/schema_test.goquery/stream.goschemas/connection.jsonschemas/profile.jsonschemas/src/connection.jsonschemas/src/connections/opentelemetry.jsonschemas/src/profile.jsonschemas/src/profiles/clickhouse.jsonschemas/src/profiles/http.jsonschemas/src/profiles/jaeger.jsonschemas/src/profiles/loki.jsonschemas/src/profiles/mysql.jsonschemas/src/profiles/opensearch.jsonschemas/src/profiles/opentelemetry.jsonschemas/src/profiles/postgres.jsonschemas/src/profiles/postgrest.jsonschemas/src/profiles/prometheus.jsonschemas/src/profiles/sql.jsonschemas/src/profiles/sqlserver.json
💤 Files with no reviewable changes (1)
- cmd/query/www/src/chatWidget.tsx
Comment on lines
+27
to
+36
| if strings.Contains(alias.Name, ".") { | ||
| aliasRoots[strings.Split(alias.Name, ".")[0]] = true | ||
| } | ||
| } | ||
| for _, ignored := range profile.Ignore { | ||
| if aliasRoots[ignored] { | ||
| continue | ||
| } | ||
| delete(row, ignored) | ||
| deleteRowPath(row, ignored) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve aliases without skipping the requested root deletion.
With alias request.xml and ignore: ["request"], aliasRoots suppresses the deletion entirely, retaining every unrelated original request.* field. Store the alias outputs, apply ignores, then restore the projected alias paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@query/cel.go` around lines 27 - 36, Update the alias filtering flow around
aliasRoots and the profile.Ignore loop so ignored roots are still deleted from
the original row even when they have aliases. Preserve the projected alias
outputs separately, apply delete(row, ignored) and deleteRowPath(row, ignored),
then restore the saved alias paths afterward so request.xml remains available
without retaining unrelated request.* fields.
Comment on lines
+198
to
+215
| func normalizeTraceTimestamp(raw string) string { | ||
| if raw == "" { | ||
| return "" | ||
| } | ||
| if number, err := strconv.ParseInt(raw, 10, 64); err == nil { | ||
| if number > 10_000_000_000_000 { | ||
| return time.Unix(0, number).Format(time.RFC3339Nano) | ||
| } | ||
| if number > 10_000_000_000 { | ||
| return time.UnixMilli(number).Format(time.RFC3339Nano) | ||
| } | ||
| return time.Unix(number, 0).Format(time.RFC3339Nano) | ||
| } | ||
| if timestamp, err := time.Parse(time.RFC3339Nano, raw); err == nil { | ||
| return timestamp.Format(time.RFC3339Nano) | ||
| } | ||
| return raw | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Test the Go timestamp parsing logic to check for microsecond misinterpretation.
cat << 'EOF' > test_ts.go
package main
import (
"fmt"
"time"
)
func main() {
// example current time in different units
now := time.Now()
ms := now.UnixMilli()
us := now.UnixMicro()
ns := now.UnixNano()
fmt.Printf("ms: %d, us: %d, ns: %d\n", ms, us, ns)
testVal := func(number int64, label string) {
var res string
if number > 10_000_000_000_000 {
res = time.Unix(0, number).Format(time.RFC3339Nano)
} else if number > 10_000_000_000 {
res = time.UnixMilli(number).Format(time.RFC3339Nano)
} else {
res = time.Unix(number, 0).Format(time.RFC3339Nano)
}
fmt.Printf("%s interpreted as: %s\n", label, res)
}
testVal(ms, "milliseconds")
testVal(us, "microseconds")
testVal(ns, "nanoseconds")
}
EOF
go run test_ts.goRepository: flanksource/commons-db
Length of output: 397
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the parser implementation around normalizeTraceTimestamp and its callers.
FILE="query/providers/opentelemetry_parser.go"
echo "== Outline =="
ast-grep outline "$FILE" --view expanded || true
echo
echo "== Relevant lines around normalizeTraceTimestamp =="
sed -n '160,260p' "$FILE" | cat -n
echo
echo "== Search for normalizeTraceTimestamp usages and format-related logic =="
rg -n "normalizeTraceTimestamp|options\.Format|Format" query/providers -SRepository: flanksource/commons-db
Length of output: 4583
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,180p' query/providers/opentelemetry.go | cat -nRepository: flanksource/commons-db
Length of output: 7304
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,60p' query/providers/opentelemetry_parser.go | cat -n
rg -n "startTimeMillis|normalizeTraceTimestamp|timestamp" query/providers -SRepository: flanksource/commons-db
Length of output: 4701
Handle microsecond timestamps explicitly. normalizeTraceTimestamp treats any value above 10_000_000_000_000 as nanoseconds, so microsecond timestamps are parsed as 1970 dates. Add a microsecond branch here, or make the conversion depend on the source format.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@query/providers/opentelemetry_parser.go` around lines 198 - 215, Update
normalizeTraceTimestamp to recognize microsecond Unix timestamps explicitly
before the nanosecond branch, converting them with the appropriate
time.Unix-based microsecond handling. Preserve the existing seconds,
milliseconds, nanoseconds, RFC3339Nano, and raw-value fallback behavior.
Replace the raw profile form with a guided workflow for selecting connections, exploring and sampling data, configuring discovered fields, and reviewing profiles before saving. Add filtering, bulk selection, field editing, provider-aware query browsing, and focused coverage for large field sets.
Capture dependent view definitions before destructive schema changes and restore unmanaged views afterward, allowing migrations to proceed without losing operator-created views. Preserve materialized-view indexes, ownership, comments, and grants during restoration while keeping failures actionable with replayable DDL.
Centralize integration-test database setup across external PostgreSQL and opt-in embedded instances. Migrate e2e, migration, and query tests to shared handles with cleanup and unique role names. Document configuration and add PostgreSQL round-trip coverage. BREAKING CHANGE: DB tests now skip unless COMMONS_DB_URL or COMMONS_DB_EMBEDDED_TEST=1 is set; external URLs create isolated databases by default unless COMMONS_DB_CREATE=false.
Exercise embedded and external PostgreSQL paths so DB integration coverage runs in CI instead of being skipped. Detect leaked scratch databases and fail the quality gate when either test job fails.
Ensure nested connections resolve by their full URL and profile mapping only accepts OpenTelemetry providers. Preserve connection ownership and mutually exclusive session settings across profile imports. Apply row transforms during sampling and correctly parse OpenTelemetry fields and duration units.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
dbtest/dbtest.go (1)
197-222: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueTemp data dir leaks when
StartEmbeddedfails.If
commonsdb.StartEmbeddederrors (Line 210-212), the temp dir created at Line 200-204 is never removed since the function returns before the cleanup closure that would remove it is ever built.♻️ Proposed fix
dsn, stop, err := commonsdb.StartEmbedded(commonsdb.EmbeddedConfig{ DataDir: dataDir, Database: sanitize(opts.Name), }) if err != nil { + if opts.DataDir == "" { + _ = os.RemoveAll(dataDir) + } return "", nil, fmt.Errorf("start embedded postgres: %w", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbtest/dbtest.go` around lines 197 - 222, Update startEmbedded so an error from commonsdb.StartEmbedded removes the temporary dataDir created when opts.DataDir is empty before returning. Preserve caller-provided directories, and keep the existing cleanup closure behavior for successful starts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e.yml:
- Around line 40-44: Add an embedded-PostgreSQL test step or job to the
e2e-tests workflow that runs the ./db/... suite while COMMONS_DB_EMBEDDED_TEST=1
is set, ensuring it executes on both supported platforms. Keep the existing
./e2e and helper test commands unchanged.
In `@dbtest/dbtest.go`:
- Line 1: Update the database setup and teardown calls in dbtest.go and
scratch.go to use bounded contexts: replace handle.Ping() with
handle.PingContext(ctx), and replace the admin/cleanup Exec calls with
ExecContext(ctx, ...). Create short-lived timeout contexts for these operations
and preserve the existing error handling and cleanup behavior.
- Around line 8-12: Update the COMMONS_DB_EMBEDDED_TEST gate in the
embedded-server startup logic to enable embedded mode only when the environment
variable equals "1"; treat unset, empty, "0", "false", and other values as
disabled. Keep the documented opt-in behavior and existing COMMONS_DB_URL
handling unchanged.
---
Nitpick comments:
In `@dbtest/dbtest.go`:
- Around line 197-222: Update startEmbedded so an error from
commonsdb.StartEmbedded removes the temporary dataDir created when opts.DataDir
is empty before returning. Preserve caller-provided directories, and keep the
existing cleanup closure behavior for successful starts.
🪄 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: 01b7566a-d0ee-4ae1-8f4f-4d4a0e1f076a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (51)
.github/workflows/e2e.yml.github/workflows/query.ymlTaskfile.yamlcmd/query/connections/service.gocmd/query/connections/service_test.gocmd/query/internal/app/migrations_test.gocmd/query/profiles/execution.gocmd/query/profiles/execution_test.gocmd/query/profiles/resolver.gocmd/query/profiles/resolver_test.gocmd/query/sessions/store_test.gocmd/query/www/src/buildProfileAction.tsxcmd/query/www/src/profileConnectionMapping.test.tscmd/query/www/src/profileConnectionMapping.tsxcmd/query/www/src/profileFieldManager.tsxcmd/query/www/src/profileWizard.test.tsxcmd/query/www/src/profileWizard.tsxcmd/query/www/src/profileWizardModel.tscmd/query/www/src/profileWizardQueryStep.tsxcmd/query/www/src/profileWizardSteps.tsxdbtest/adapters.godbtest/dbtest.godbtest/scratch.godbtest/scratch_test.goe2e/README.mde2e/helpers/services.goe2e/postgres_test.gogo.modlogs/opensearch/types.gomigrate/integration_test.gomigrate/migrate.gomigrate/migrate_test.gomigrate/scripts.gomigrate/view_deps.gomigrate/view_deps_integration_test.gomigrate/view_deps_test.gomigrate/viewdeps/capture.gomigrate/viewdeps/drop.gomigrate/viewdeps/viewdeps.gomigrate/viewdeps/viewdeps_integration_test.gomigrate/viewdeps/viewdeps_suite_test.gomigrate/viewdeps/viewdeps_test.goquery/cel.goquery/engine_test.goquery/providers/new_providers_test.goquery/providers/opentelemetry.goquery/providers/opentelemetry_parser.goquery/providers/opentelemetry_parser_test.goquery/providers/sql_integration_test.goquery/sample.goquery/sample_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- cmd/query/www/src/profileConnectionMapping.test.ts
- migrate/migrate_test.go
- query/engine_test.go
- cmd/query/profiles/resolver.go
- cmd/query/connections/service_test.go
- cmd/query/profiles/execution_test.go
- cmd/query/www/src/profileConnectionMapping.tsx
- query/providers/new_providers_test.go
- query/cel.go
- migrate/scripts.go
- query/providers/opentelemetry.go
- query/providers/opentelemetry_parser.go
- cmd/query/profiles/execution.go
Comment on lines
+40
to
+44
| # Exercises the embedded-postgres path on both platforms. The external-DB | ||
| # path is covered by e2e-external, which needs a Linux-only service container. | ||
| env: | ||
| COMMONS_DB_EMBEDDED_TEST: "1" | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for other workflows that might already cover this, and confirm db/embedded_test.go's env gating.
fd -e yml -e yaml . .github/workflows
rg -n 'COMMONS_DB_EMBEDDED_TEST' -g '*.go' -g '*.yml' -g '*.yaml'Repository: flanksource/commons-db
Length of output: 276
e2e-tests still skips the embedded DB suite.
COMMONS_DB_EMBEDDED_TEST=1 is set here, but this job only runs ./e2e and ./e2e/helpers/.... The ./migrate/..., ./query/..., and ./db/... tests only run in e2e-external, so the embedded-only paths gated by COMMONS_DB_EMBEDDED_TEST never execute in CI. If the goal is to cover embedded PostgreSQL on both platforms, this needs a job that runs ./db/... with that env var set.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e.yml around lines 40 - 44, Add an embedded-PostgreSQL
test step or job to the e2e-tests workflow that runs the ./db/... suite while
COMMONS_DB_EMBEDDED_TEST=1 is set, ensuring it executes on both supported
platforms. Keep the existing ./e2e and helper test commands unchanged.
| @@ -0,0 +1,222 @@ | |||
| // Package dbtest resolves the PostgreSQL database that integration tests run | |||
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Bare Ping/Exec calls with no context can hang test setup/teardown indefinitely. Both sites perform blocking Postgres calls without a context or timeout, so a slow/unreachable server stalls tests instead of failing fast (confirmed by golangci-lint's noctx hint on both).
dbtest/dbtest.go#L79-99: usehandle.PingContext(ctx)with a bounded context (e.g. a short-livedcontext.WithTimeout) instead ofhandle.Ping().dbtest/scratch.go#L30-63: useadmin.ExecContext(ctx, ...)/cleanup.ExecContext(ctx, ...)with a bounded context instead of the bareExeccalls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dbtest/dbtest.go` at line 1, Update the database setup and teardown calls in
dbtest.go and scratch.go to use bounded contexts: replace handle.Ping() with
handle.PingContext(ctx), and replace the admin/cleanup Exec calls with
ExecContext(ctx, ...). Create short-lived timeout contexts for these operations
and preserve the existing error handling and cleanup behavior.
Comment on lines
+8
to
+12
| // COMMONS_DB_URL connect to this server instead of embedding one | ||
| // COMMONS_DB_CREATE "false" uses COMMONS_DB_URL as-is; anything else | ||
| // (including unset) carves out a fresh database | ||
| // COMMONS_DB_EMBEDDED_TEST "1" permits starting an embedded server when | ||
| // COMMONS_DB_URL is unset |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
COMMONS_DB_EMBEDDED_TEST gate accepts any non-empty value, not just "1" as documented.
The package doc and downstream callers (e.g. query/providers/sql_integration_test.go) document the flag as COMMONS_DB_EMBEDDED_TEST=1, but the switch only skips when the value is empty — setting it to "false" or "0" (an intuitive way to disable it) will actually start an embedded server, defeating the "opt-in gate" the doc itself explains is needed to avoid exhausting a machine's SysV shared memory.
🐛 Proposed fix
- case os.Getenv(EnvEmbedded) == "":
+ case os.Getenv(EnvEmbedded) != "1":
return nil, nil, ErrSkipAlso applies to: 166-175
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dbtest/dbtest.go` around lines 8 - 12, Update the COMMONS_DB_EMBEDDED_TEST
gate in the embedded-server startup logic to enable embedded mode only when the
environment variable equals "1"; treat unset, empty, "0", "false", and other
values as disabled. Keep the documented opt-in behavior and existing
COMMONS_DB_URL handling unchanged.
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
1 participant
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.
What
Notes
Summary by CodeRabbit
New Features
Bug Fixes