Conversation
There was a problem hiding this comment.
Pull request overview
Adds YAML-based configuration support for sql-tapd, allowing runtime settings to be loaded from a config file and overridden by CLI flags.
Changes:
- Introduces a
configpackage with defaults and a YAML loader (with unknown-field detection). - Updates
sql-tapdto load config from.sql-tap.yaml(or-config) and apply CLI overrides only for explicitly-set flags. - Extends golangci-lint tag style rules to enforce
yamltags assnake_case.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
config/config.go |
New config model, defaults, and YAML loader with KnownFields(true) to catch typos. |
config/config_test.go |
Unit tests for defaults, explicit/implicit loading behavior, partial overrides, and invalid/unknown YAML. |
cmd/sql-tapd/main.go |
Wires config loading into sql-tapd, implements “explicit flag overrides config” semantics, and refactors run signature. |
.golangci.yaml |
Enforces snake_case for YAML struct tags via tagliatelle. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| driver := fs.String("driver", "", "database driver: postgres, mysql, tidb (required)") | ||
| listen := fs.String("listen", "", "client listen address (required)") | ||
| upstream := fs.String("upstream", "", "upstream database address (required)") |
There was a problem hiding this comment.
The flag descriptions for -driver/-listen/-upstream still say "(required)", but this PR also allows providing those values via the config file. Consider updating the help text to reflect that they are only required when not set in the config (e.g., "required unless provided by -config / .sql-tap.yaml").
| driver := fs.String("driver", "", "database driver: postgres, mysql, tidb (required)") | |
| listen := fs.String("listen", "", "client listen address (required)") | |
| upstream := fs.String("upstream", "", "upstream database address (required)") | |
| driver := fs.String("driver", "", "database driver: postgres, mysql, tidb (required unless provided by -config / .sql-tap.yaml)") | |
| listen := fs.String("listen", "", "client listen address (required unless provided by -config / .sql-tap.yaml)") | |
| upstream := fs.String("upstream", "", "upstream database address (required unless provided by -config / .sql-tap.yaml)") |
1e74dc1 to
8b7f306
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cmd/sql-tapd/main.go:141
explainDriveris left as the zero value whencfg.Driveris not one of mysql/tidb/postgres, which maps toexplain.Postgres(Driver iota=0). That means an unsupported driver can incorrectly initialize the EXPLAIN client as Postgres before later failing in the proxy switch. Consider adding adefaultcase that returns an error (or validatingcfg.Driveronce up-front) so unsupported drivers don’t silently fall back to Postgres behavior.
var explainDriver explain.Driver
switch cfg.Driver {
case "mysql":
explainDriver = explain.MySQL
case "tidb":
explainDriver = explain.TiDB
case "postgres":
explainDriver = explain.Postgres
}
explainClient = explain.NewClient(db, explainDriver)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.