Add a compatibility page and pin the exported API surface - #244
Conversation
github-actions
Bot
added
documentation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7fb304c93
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| All three protocols share one keyspace, and that stays true: a value written over HTTP is | ||
| readable over RESP and gRPC, and the reverse. |
There was a problem hiding this comment.
Limit this promise to string values: RESP supports lists, hashes, sets, and sorted sets, but the HTTP and gRPC GET handlers return an internal error when the stored value is not a string. For example, after LPUSH key value, neither HTTP nor gRPC can read key, so the stated reverse interoperability is already false.
Useful? React with 👍 / 👎.
|
|
||
| ## Trust boundary | ||
|
|
||
| **kvs has no authentication, no authorization, and no TLS.** Anyone who can open a connection |
There was a problem hiding this comment.
Describe the listeners separately rather than saying kvs has no authentication: setting resp_password or KVS_RESP_PASSWORD makes the RESP server require AUTH, and cluster join requests authenticate through that listener as well. The statement is therefore false whenever an operator configures the already-documented RESP password; only HTTP and gRPC are always unauthenticated.
Useful? React with 👍 / 👎.
|
|
||
| func (e *NotLeaderError) Is(target error) bool | ||
|
|
||
| func (s *Store) SetReplicator(replicate func(fn func(tx *Tx) error) error) |
There was a problem hiding this comment.
Remove the exempt plumbing methods from this promised golden surface, or track them separately. The compatibility page explicitly allows SetReplicator, ReplaceWith, and ApplyReplicated to change or disappear in a v1 minor release, but their presence here makes TestPublicAPISurface fail such a permitted change with the message that it requires a major version.
Useful? React with 👍 / 👎.
|
|
||
| fmt.Fprintf(&buf, "==== %s ====\n", name) | ||
|
|
||
| if err := printer.Fprint(&buf, fset, file); err != nil { |
There was a problem hiding this comment.
Normalize receiver and parameter identifiers before printing the snapshot. Those identifiers are not part of Go source compatibility, but the generated text includes names such as s, tx, and fn, so a harmless rename like s to store fails this test and is reported as a breaking change requiring a major version.
Useful? React with 👍 / 👎.
| case *ast.GenDecl: | ||
| if d.Tok == token.IMPORT { | ||
| continue |
There was a problem hiding this comment.
Keep enough import information to identify qualified types. After this block removes imports, a dependency migration from one import path to another package with the same local package name leaves signatures such as pkg.Type textually unchanged, even though callers see a distinct Go type and may no longer compile; the compatibility guard therefore misses that breaking change.
Useful? React with 👍 / 👎.
| Every flag on the [CLI Usage](../cli/) page keeps its name, its meaning, and its default, | ||
| along with the config-file key and environment variable that set it. `kvs serve` with no flags | ||
| keeps listening on `:3456` for HTTP, `:3457` for gRPC, and `127.0.0.1:6379` for RESP. |
There was a problem hiding this comment.
State this as the default address rather than promising that the process keeps listening there. When 127.0.0.1:6379 is already occupied, the branch in internal/server/run.go:80-90 deliberately treats the default RESP bind failure as nonfatal, logs that RESP is off, and continues with only HTTP and gRPC, so kvs serve with no flags does not provide the promised listener in that scenario.
Useful? React with 👍 / 👎.
|
Codex review, six comments. Three were right about the page and are fixed in 2eab847.
The other three I did not take:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2eab847f27
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
State in one place what will not break and what deliberately might, so a reader deciding whether to depend on kvs can answer that from a page rather than from the source. A promise nobody checks rots, so the Go half of it is enforced: TestPublicAPISurface renders every exported declaration in the root package and compares it against testdata/api-surface.txt. A symbol cannot join or leave without the diff landing in review. Comments are left unparsed and function bodies dropped, which keeps the golden file about signatures and free of everything else. SetReplicator, ReplaceWith, and ApplyReplicated are exported so internal/cluster can reach them across the package boundary, and pkg/resp exists so the server can speak RESP2 rather than as a library for other programs. Both now say so where someone reading godoc will find it. The page also writes down the trust boundary as it stands: no authentication, no authorization, no TLS, and HTTP and gRPC listening on every interface by default while RESP stays on loopback.
The page said all three protocols read each other's values, that kvs has no authentication at all, and that `kvs serve` keeps listening on the default RESP address. None of the three holds: HTTP and gRPC report an error for a key holding a RESP list, hash, set, or sorted set; `resp_password` makes the RESP listener require AUTH before anything else, cluster joins included; and a default RESP port already taken is logged and skipped rather than being fatal. The golden surface keeps the exempt cluster plumbing, because it records what the package exports rather than what v1 promises, and its header now says where the exemptions are written down.
The HTTP API page says all errors return a JSON body and lists 405 among them, but both method checks answered with a bare WriteHeader: no body, no JSON content type. The compatibility page now makes those response bodies part of v1, so this is the last release where the empty one can be corrected rather than kept. The 405 path had no test at all, which is how it stayed wrong.
The README repeated the claim that kvs has no authentication, which the compatibility page had already stopped making: `resp_password` gives the RESP listener an AUTH requirement, and only HTTP and gRPC are unauthenticated whatever the operator does. The command-line promise covered every flag on the CLI page, including the config-file key and environment variable that set it. `--config`, `--version`, and `--help` have neither, because the CLI answers them itself rather than passing them to the server, so that half of the promise is now scoped to the `serve` settings that do have Viper mappings. The CLI page overstated the same thing and now says `serve` too.
c9268bc to
67cc72c
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67cc72c83c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func (h *httpHandler) handleHealth(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method != http.MethodGet { | ||
| w.WriteHeader(http.StatusMethodNotAllowed) | ||
| writeJSONError(w, http.StatusMethodNotAllowed, "method not allowed") |
There was a problem hiding this comment.
When a client sends HEAD /healthz or HEAD /v1/keys/key, this branch selects the new 405 JSON response, but Go's net/http transport suppresses response bodies for HEAD requests. The compatibility page now promises response bodies and the HTTP API says every error has JSON, while the added test exercises only POST through httptest.ResponseRecorder, so real HEAD clients still receive an empty 405; either narrow the documented guarantee for HEAD or handle it explicitly.
Useful? React with 👍 / 👎.
| // FileExports drops unexported declarations, struct fields, and interface methods. | ||
| // It keeps the bodies of the exported functions, which is why signaturesOnly runs | ||
| // after it: how a method is written is not what callers depend on. | ||
| if !ast.FileExports(file) { |
There was a problem hiding this comment.
Because ast.FileExports removes unexported fields, the snapshot records ReadTx only as an opaque struct even though its current fields make the exported type comparable. If a later minor release adds an unexported slice, map, or function field, downstream code that compares ReadTx values or uses them as map keys stops compiling, but this guard produces no diff. Retain enough type information to record comparability, or type-check exported structs and include that property in the snapshot.
Useful? React with 👍 / 👎.
| Every flag on the [CLI Usage](../cli/) page keeps its name, its meaning, and its default. Each | ||
| `kvs serve` setting keeps the config-file key and environment variable that set it too; |
There was a problem hiding this comment.
--join or follow leader redirects
When --join points to a follower, which the CLI page's “a node already in the cluster” wording and the README's “any existing node” wording explicitly allow, that node answers MOVED, but requestJoin only calls expectOK and joinCluster retries the same follower forever instead of following the supplied leader address. The new guarantee that every flag retains its documented meaning therefore promises a join flow that does not work in this common failover scenario; either follow MOVED or restrict the promised target to the current leader.
Useful? React with 👍 / 👎.
1 participant
Summary
kvs exposes three wire protocols, a CLI, and a Go package, and nothing said which of them a
future release would keep. This adds the page that says so, and a test that keeps the Go half
of the answer honest.
No behaviour changes.
Type of Change
Changes
content/docs/compatibility.md— what av1tag will promise not to break (HTTP, gRPC,RESP2, the CLI flags, and the root Go package), what it deliberately leaves out, and how
something gets removed if it has to be. It also writes down the trust boundary as it stands:
no authentication, no authorization, no TLS, HTTP and gRPC on every interface by default
while RESP stays on loopback.
api_surface_test.goandtestdata/api-surface.txt—TestPublicAPISurfacerenders everyexported declaration in the root package and compares it against the golden file, so a
symbol cannot join or leave without the diff landing in review. Comments are left unparsed
and function bodies dropped, which keeps the file about signatures and nothing else.
Regenerate deliberately with
go test -run TestPublicAPISurface . -update.replication.goandpkg/resp/reader.go—SetReplicator,ReplaceWith, andApplyReplicatedare exported sointernal/clustercan reach them across the packageboundary, and
pkg/respexists so the server can speak RESP2 rather than as a library forother programs. Both now say so in godoc.
README.md,content/docs/overview.md— link to the new page.contributing.mdandrelease.mdshift one place in the sidebar to make room for it.Testing
make allpassesChecked that the guard actually guards: adding a dummy exported function fails
TestPublicAPISurfacewith the offending line, and removing it passes again.Related Issues
None.