Skip to content

refactor(config): replace frozen protobuf config with plain Go - #150

Open
Cianidos wants to merge 4 commits into
mainfrom
refactor/issue-143-plain-go-config
Open

refactor(config): replace frozen protobuf config with plain Go#150
Cianidos wants to merge 4 commits into
mainfrom
refactor/issue-143-plain-go-config

Conversation

@Cianidos

Copy link
Copy Markdown
Contributor

Closes #143

Replaces the frozen protobuf application configuration types under pkg/common/proto/stroppy with plain-Go structs and enums in a new pkg/config package. The protobuf/gRPC dependencies for YDB and OTLP remain.

Acceptance criteria

  • Plain Go types for run / workload / driver / pool / logger / exporter / isolationpkg/config defines RunConfig, DriverRunConfig, DriverConfig, PoolConfig, PostgresConfig, SqlConfig, InsertProgressConfig, GlobalConfig, LoggerConfig, ExporterConfig, OtlpExport, and the DriverType / ErrorMode / TxIsolationLevel / LogLevel / LogMode enums.
  • Preserve JSON field names + strict unknown-field rejection — camelCase json tags are carried over verbatim; protojson is replaced by encoding/json with DisallowUnknownFields (internal/runner.UnmarshalStrict).
  • Replace protojson / proto.Merge — explicit JSON decode plus a deterministic per-field merge (internal/runner.MergePostgresConfig / MergeSqlConfig) that only overrides non-nil fields, unit-tested.
  • Source precedence + presence — optional scalars stay *T pointers; the typed precedence CLI > env > -e > typed config > config env > default is untouched.
  • jsonschema from the Go contractdocs/jsonschema/run.schema.json is now generated by the reflection-based internal/jsonschema-gen (go generate ./pkg/config), with no stale proto/TS descriptions.
  • Migrate runners / drivers / bench / tests — all call sites updated; bench.Run, Workload, and driver.Driver public surfaces are unchanged apart from the config type.
  • Deletionspkg/common/proto/stroppy (all .pb.go + .pb.validate.go + descriptors), dead runtime.pb.go value types (Value, Decimal, Uuid, DateTime, DriverQuery, stats, etc.), pkg/utils/protovalue, pkg/utils/protoyaml, pkg/common/logger/proto.go, and the protoc-gen-validate dependency are removed.
  • YDB/OTLP dependencies keptgoogle.golang.org/protobuf and google.golang.org/grpc remain (indirect) for those external protocols.
  • Golden tests — accepted and rejected JSON (unknown fields, wrong types) in pkg/config/config_test.go plus the merge unit tests in internal/runner/merge_test.go.

Migration notes (v5 → v6)

  • The config file JSON shape is unchanged: existing v5 stroppy-config.json files load as before with the same camelCase field names.
  • -D/-d driver options keep identical precedence (-D postgres.* still overrides -D pool.*).
  • k6Args / k6Config remain accepted-but-inert (they had no effect already).
  • The frozen .pb.go config types are gone; any out-of-tree code importing pkg/common/proto/stroppy must switch to github.com/stroppy-io/stroppy/pkg/config.

Verification

  • make build
  • go build ./...
  • go vet ./...
  • full go test ./... ✓ (all packages green)
  • go test -race on every migrated package ✓
  • gofmt clean; go mod tidy compiles with YDB/OTLP still importing

Note: the repo-wide make tests (-race + coverage) times out at ~10 min in pkg/datagen/tpchgen (TestOfficialAnswersQ1Q6SF1, the canonical TPC-H SF=1 official-answer generator, which is untouched by this PR and independent of pkg/config); every migrated package passes -race individually.

Replace the frozen protobuf application configuration types under
pkg/common/proto/stroppy with plain-Go structs and enums in pkg/config.

- Define plain-Go types for run, driver, pool, logger, exporter, and
  isolation config, preserving the exact camelCase JSON field names and
  omitempty semantics.
- Replace protojson and proto.Merge with strict encoding/json decoding
  (DisallowUnknownFields) and a deterministic per-field pool merge in
  internal/runner.
- Generate docs/jsonschema/run.schema.json from the Go structs via a
  small reflection generator (internal/jsonschema-gen) with no stale
  proto/TS descriptions.
- Migrate runner, drivers, bench APIs, workloads, and tests; keep the
  bench.Run/Workload/Driver surfaces stable.
- Delete pkg/common/proto/stroppy, pkg/utils/protovalue,
  pkg/utils/protoyaml, and the unused pkg/common/logger proto helpers.
- Drop the protoc-gen-validate dependency; YDB/OTLP protobuf and gRPC
  dependencies remain.

Closes #143
@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@Cianidos, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 14 minutes

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c2cb5785-87b6-477e-92c8-439bab60f6c4

📥 Commits

Reviewing files that changed from the base of the PR and between c338c2f and 1e98265.

⛔ Files ignored due to path filters (6)
  • go.sum is excluded by !**/*.sum
  • pkg/common/proto/stroppy/common.pb.go is excluded by !**/*.pb.go
  • pkg/common/proto/stroppy/config.pb.go is excluded by !**/*.pb.go
  • pkg/common/proto/stroppy/descriptor.pb.go is excluded by !**/*.pb.go
  • pkg/common/proto/stroppy/run.pb.go is excluded by !**/*.pb.go
  • pkg/common/proto/stroppy/runtime.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (52)
  • .golangci.yml
  • CHANGELOG.md
  • cmd/stroppy/commands/help/topic_config_file.go
  • cmd/stroppy/commands/run/metrics_test.go
  • cmd/stroppy/commands/run/run.go
  • cmd/stroppy/commands/run/run_test.go
  • docs/jsonschema/run.schema.json
  • go.mod
  • internal/jsonschema-gen/main.go
  • internal/runner/config_file.go
  • internal/runner/config_file_test.go
  • internal/runner/driver_file_config_test.go
  • internal/runner/driver_preset.go
  • internal/runner/merge.go
  • internal/runner/merge_test.go
  • internal/runner/run_config_merge.go
  • internal/runner/run_config_merge_test.go
  • internal/workloads/tpcc/params_test.go
  • internal/workloads/tpcds/params_test.go
  • pkg/bench/enums.go
  • pkg/bench/metrics.go
  • pkg/bench/metrics_test.go
  • pkg/bench/query.go
  • pkg/bench/runtime.go
  • pkg/bench/runtime_test.go
  • pkg/common/logger/proto.go
  • pkg/common/proto/stroppy/common.pb.validate.go
  • pkg/common/proto/stroppy/config.pb.validate.go
  • pkg/common/proto/stroppy/descriptor.pb.validate.go
  • pkg/common/proto/stroppy/run.pb.validate.go
  • pkg/common/proto/stroppy/runtime.pb.validate.go
  • pkg/config/config.go
  • pkg/config/config_test.go
  • pkg/driver/csv/driver.go
  • pkg/driver/csv/driver_test.go
  • pkg/driver/csv/run_query.go
  • pkg/driver/dispatcher.go
  • pkg/driver/insert_methods.go
  • pkg/driver/insert_methods_test.go
  • pkg/driver/mysql/driver.go
  • pkg/driver/noop/driver.go
  • pkg/driver/noop/insert_test.go
  • pkg/driver/picodata/driver.go
  • pkg/driver/postgres/driver.go
  • pkg/driver/postgres/pool/pool.go
  • pkg/driver/postgres/pool/pool_test.go
  • pkg/driver/postgres/tx.go
  • pkg/driver/sqldriver/config.go
  • pkg/driver/sqldriver/tx.go
  • pkg/driver/ydb/driver.go
  • pkg/utils/protovalue/value.go
  • pkg/utils/protoyaml/protoyaml.go
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/issue-143-plain-go-config

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Reject unknown LogLevel/LogMode values on decode (the frozen protobuf
schema encoded them as strict enums), pin the remaining protojson field
names and log-enum rejection in tests, derive the JSON Schema enum values
from the exported constant lists, and note the global.seed quoted-number
divergence in the changelog.
Exempt pkg/config camelCase json tags from tagliatelle (they are the
protojson wire format), rename Go identifiers to idiomatic initialisms
(SQL, URL, TLS, RunID, OtlpHTTP*) that revive var-naming flags while
preserving the json tags, and fix the remaining gocritic/gosec/wsl/errcheck/
intrange/nlreturn/err113 findings in pkg/config and the jsonschema generator.
@Cianidos

Copy link
Copy Markdown
Contributor Author

Review blocked: maintainer decision required

Independent code/config/runtime reviewers and an adversarial verifier tested this PR after merging current main locally. Full build, lint, race tests, and PostgreSQL Docker paths passed, but the config migration has verified compatibility defects. No integration commit was pushed.

Decision required: legacy protobuf field names

v5 accepts both canonical lower-camel names and protobuf source names such as driver_type, bulk_size, max_conns, run_id, log_level, stall_after, and no_steps. This PR accepts only canonical camelCase.

Both outcomes are valid but functionally different:

  1. Preserve v5 aliases: add exact, collision-aware normalization for every former protobuf source-name alias at every nesting level, and reject canonical/alias collisions.
  2. Make v6 camelCase-only: explicitly document this additional breaking change in changelog/help/schema guidance and add comprehensive rejection/migration tests.

Issue #143 permits v6 breaks, while this PR's changelog says existing v5 files continue to load and names only global.seed as divergent. That does not determine which policy is intended. Please choose one before this PR is merged.

Reproduced defects to fix after that decision

  • Nested strict JSON validation regressed. Duplicate nested keys such as two drivers[0].url fields now silently use the last value; wrong-case keys, null driver entries, null map/list values, and nested null strings are also accepted/coerced where v5 rejects them. Strict validation must cover the full JSON tree and raw -d, report exact paths, and account for the alias policy above.
  • Generated schema rejects valid config. docs/jsonschema/run.schema.json marks the root closed but omits accepted run and params properties. Restore the effective loaded-config envelope and validate the documented typed example in tests.
  • ProtoJSON-compatible integers regressed. Existing quoted and integral-decimal/exponent forms such as bulkSize:"20" and pool.maxConns:"3" load on v5 but fail here. Preserve those forms for non-seed int32 fields while rejecting fractions, overflow, and malformed values; the documented numeric-only global.seed break can remain.
  • Numeric logger enums regressed. v5 accepts known numeric ordinals for logLevel and logMode; this PR accepts strings only. Decode both defined symbolic names and valid ordinals, with boundary/error tests.
  • Active project guidance is stale. AGENTS.md and Makefile still direct contributors to deleted pkg/common/proto/stroppy; point config guidance to pkg/config and describe remaining protobuf users accurately.

Representative evidence was reproduced against both current main (5005aa3) and the locally integrated PR (e6f1409), including real CLI config loads and schema validation. Per the sequential merge rule, this PR is skipped until the policy decision and fixes are addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace frozen protobuf configuration types with plain Go

1 participant