CXH-2381: document Db2 setup, correct the supported-database list, and fix the help link - #150
Conversation
Add a "Writing a Db2 spec" section to docs/db2.md (wrap every column reference in string(), alias columns to double-quoted lowercase), notes that account provisioning is unavailable and group principals can't be represented, and a "Running the Db2 tests" section with make test-db2 / make vet-db2 and the raw CGO_CFLAGS/CGO_LDFLAGS + library-path invocation. Add test-db2 and vet-db2 Makefile targets scoping the CGO flags to the recipe. Reconcile the engine lists (README, docs/docs-info.md, test/README.md) so they agree with the dispatch switch in pkg/database/database.go, and drop the unfilled template sentence from docs-info.md. Point the connector help URL at the live docs page (/docs/baton/baton-sql); the old /docs/baton/sql returns 404. Bump the ci.yaml sync-test action to @v4 and gitignore the root baton-sql build output.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ConfigurationFields, | ||
| field.WithConnectorDisplayName("SQL"), | ||
| field.WithHelpUrl("/docs/baton/sql"), | ||
| field.WithHelpUrl("/docs/baton/baton-sql"), |
There was a problem hiding this comment.
🟡 Suggestion: every other ConductorOne connector uses /docs/baton/<slug> without the baton- prefix (/docs/baton/slack, /docs/baton/box, /docs/baton/github-v2); an org-wide code search for "/docs/baton/baton-" returns 0 hits. Worth confirming the docs page actually lives at /docs/baton/baton-sql rather than /docs/baton/sql-connector or similar, since this is the one link the app surfaces to customers. (Medium confidence — I could not reach the docs site from CI to verify.)
| run: go build ./cmd/baton-sql | ||
| - name: Run sync tests | ||
| uses: ConductorOne/github-workflows/actions/sync-test@v2 | ||
| uses: ConductorOne/github-workflows/actions/sync-test@v4 |
There was a problem hiding this comment.
🟡 Suggestion: this major bump isn't mentioned in the PR description, and its main new capability is a no-op here. sync-test@v4 adds an auth-error.sh step that only invalidates BATON_* env vars matching *TOKEN*|*PASSWORD*|*SECRET*|*KEY*|*CREDENTIAL*|*PAT|*PASSWD*|*BEARER*. This job's credential is DB_PASSWORD (not BATON_*), and BATON_LOG_LEVEL/BATON_CONFIG_PATH/BATON_PROVISIONING are all filtered out, so the script finds nothing to invalidate and exits 0 with a skip notice. Adding bad-credentials: DB_PASSWORD=invalid would actually exercise the unauthenticated-sync check. (CI is green either way — this is lost coverage, not a break.)
Connector PR Review: CXH-2381: document Db2 setup, correct the supported-database list, and fix the help linkBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review Summary The new commit is docs-only: it softens the README key-features line to point at the engine list, adds the Oracle/Redshift contrast to the Db2 spec-authoring section, corrects the Security Issues None found. Correctness Issues None found. Suggestions None. |
…h check Add WordPress (MySQL-based) back to the README engine list so it agrees with docs/docs-info.md, which still ships it. Pass bad-credentials: DB_PASSWORD=invalid to sync-test@v4 so the new auth-error check actually runs; without it the step skips (DB_PASSWORD is not a BATON_* var, so nothing gets invalidated).
| baton-entitlement: 'role:admin:member' | ||
| baton-principal: john.smith | ||
| baton-principal-type: user | ||
| bad-credentials: DB_PASSWORD=invalid |
There was a problem hiding this comment.
🟡 Suggestion: This gets auth-error.sh past the "no credential env vars found" skip, but the check still won't assert anything. cmd/baton-sql/main.go uses the deprecated configSdk.DefineConfiguration + os.Exit(1) pattern rather than config.RunConnector/exit.LogExit, so an invalid DB_PASSWORD exits 1 — which auth-error.sh treats as "not migrated yet, skipping" and passes. Switching main.go to config.RunConnector (available in baton-sdk v0.28.0) would surface gRPC Unauthenticated/PermissionDenied and make this input actually exercise the check.
true
|
|
||
| ## Writing a Db2 spec | ||
|
|
||
| Db2 needs two things in every spec that the other engines don't. Both fail loudly, but the |
There was a problem hiding this comment.
[Review] this isn't actually Db2-only, other examples already do this
examples/oracle-test.yml:120-121 already double-quote-aliases columns to lowercase (Oracle uppercase-folds unquoted identifiers too), and examples/redshift-test.yml:266-267 already wraps column refs in string() everywhere. Funniest part: two lines down you literally tell people to go copy examples/redshift-test.yml's use of this exact pattern lol, which kinda contradicts "the other engines don't" right there in the same section.
Maybe reframe as something like "Db2 needs this everywhere, other engines only need it in spots (Oracle's identifier folding, Redshift's CEL concatenations)" instead of implying it's unique to Db2?
| group membership is reachable only through the per-authorization-ID function | ||
| `SYSPROC.AUTH_LIST_GROUPS_FOR_AUTHID`, which a YAML resource list can't express. A grant | ||
| emitted with `principal_type: group` is dropped at ingest (visible as `grants_dropped` and | ||
| `ingest_quality.reason_flags` in the sync token), and `group` is omitted from `grantableTo` |
There was a problem hiding this comment.
[Review] pretty sure this isn't actually enforced in code
Checked pkg/bsql/entitlements.go (the GrantableTo builder, both the static ~L69-75 and dynamic ~L180-186 versions) and there's zero s.dbEngine awareness anywhere in there — it's 100% spec-driven. So if a Db2 spec declares group in grantable_to, it'll show up in grantableTo verbatim, nothing filters it out.
I think what's actually happening is you're conflating two different things: grantableTo (entitlement metadata, spec-driven, not touched by engine) vs the grant itself getting dropped at ingest time (the grants_dropped/ingest_quality.reason_flags thing, which is real and does happen). Might be worth splitting those into two separate sentences so it's clear grantableTo will still say group is allowed even though any actual group grant gets silently dropped.
| > - SAP HANA | ||
| > - Vertica | ||
| > - Amazon Redshift | ||
| > - IBM DB2 (opt-in; requires a binary built with the `db2` tag — see [docs/db2.md](db2.md)) |
There was a problem hiding this comment.
[Review] nice fix here, but two other lists in this same file still need it too
This engine list is now accurate, but scrolling down in docs-info.md:
- the "Example DSN formats" list is still just MySQL/PostgreSQL/Oracle/SQL Server
- the "Configuration Examples" list at the bottom still only mentions
mysql-test.yml/postgres-test.yml/oracle-test.yml/wordpress-test.yml/sqlserver-test.yml, even thoughexamples/redshift-test.yml,examples/sap-hana-test.yml, andexamples/vertica-test.ymlalready exist in the repo
Kinda funny that this is literally the exact bug class this PR is fixing, just still lurking two sections down in the same doc. Might be worth a quick pass while you're in here.
| ## Key Features | ||
|
|
||
| - **Multi-Database Support**: Works with MySQL, PostgreSQL, Oracle, SQL Server, Vertica, SQLite, and WordPress | ||
| - **Multi-Database Support**: Works with MySQL, PostgreSQL, Oracle, SQL Server, SAP HANA, Vertica, and Amazon Redshift, plus opt-in IBM DB2 (see [Supported Database Engines](#supported-database-engines)) |
There was a problem hiding this comment.
[Review] this line and the list below it disagree on WordPress
This summary line drops WordPress, but the "Supported Database Engines" list a few lines down (line 29) has "WordPress (MySQL-based)" — looks like it got restored there in the follow-up commit but this summary line never got the same treatment. WordPress is real (rides mysql://, has its own example + docs elsewhere), so this isn't a SQLite-style correct removal, it's just drift between two lists that are supposed to say the same thing.
Honestly since this line already links to #supported-database-engines, might be simpler to just say something like "Works with a wide range of SQL databases — see Supported Database Engines below" instead of hand-maintaining the same enumeration twice. Would've avoided this exact drift.
- db2.md: reframe the "Writing a Db2 spec" intro. The string()-wrapping and lowercase-alias patterns are not Db2-only; Oracle folds unquoted identifiers and Redshift needs string() in CEL concatenations. Db2 just needs both everywhere. - db2.md: correct the group-provisioning note. grantableTo is spec-driven and is not filtered by engine, so a spec that declares group still advertises it as grantable; the group grant is dropped at ingest, not at grantableTo. Split the two into separate statements. - docs-info.md: bring the "Example DSN formats" and "Configuration Examples" lists in line with the supported-engines list (SAP HANA, Vertica, Amazon Redshift, IBM DB2). - README.md: drop the hand-maintained engine enumeration in Key Features that had drifted from the Supported Database Engines list on WordPress; link to that list instead.
3 participants
Documents how to set up the IBM Db2 database engine for the SQL connector, corrects the list of supported databases so it matches what the connector actually supports, and fixes the connector's help link, which pointed at a dead page.