Skip to content

feat(gateway): land and status subcommands for the client - #547

Merged
behinddwalls merged 2 commits into
mainfrom
sq/gateway-land-cli
Aug 13, 2026
Merged

feat(gateway): land and status subcommands for the client#547
behinddwalls merged 2 commits into
mainfrom
sq/gateway-land-cli

Conversation

@behinddwalls

@behinddwalls behinddwalls commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

Why?

The gateway client only knew how to ping. Submitting a change meant hand-assembling protobuf with grpcurl, including a 40-character commit SHA copied out of a pull request — which is both tedious and easy to get subtly wrong.

What?

ping | land | status subcommands. land takes repeated -uri flags, whose order is significant: several changes in one request are a stack, applied in the order given.

-pr <url> resolves a pull request's head commit and mints the change URI, so nobody types a SHA. It dispatches on the change URL the same way the merger and the change provider dispatch on the URI scheme, rather than assuming one provider; GitHub is implemented, and adding another is a case beside it. Reading a public repository needs no token, and a 404 without one says so rather than reporting the pull request as missing.

Test Plan

go build ./service/submitqueue/gateway/client and go vet.

✅ Manually: land -queue q reports the missing-change error; land -pr https://github.com/uber/cadence/pull/1 resolved against the live GitHub API and printed github://github.com/uber/cadence/pull/1/65c322d4... before failing to dial a gateway, which is the expected end of that path with no stack running.

Issues

@behinddwalls
behinddwalls marked this pull request as ready for review August 7, 2026 18:24
@behinddwalls
behinddwalls requested review from a team and sbalabanov as code owners August 7, 2026 18:24
@kevinlnew

Copy link
Copy Markdown
Contributor

The diff includes a compiled binary client at the repo root:

diff --git a/client b/client
new file mode 100755
index 00000000..5db01cb2
Binary files /dev/null and b/client differ

Looks accidental — should be removed and probably added to .gitignore.

mnoah1
mnoah1 previously requested changes Aug 12, 2026

@mnoah1 mnoah1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the committed client (18mb) intentional here?

@behinddwalls

Copy link
Copy Markdown
Collaborator Author

The diff includes a compiled binary client at the repo root:

diff --git a/client b/client
new file mode 100755
index 00000000..5db01cb2
Binary files /dev/null and b/client differ

Looks accidental — should be removed and probably added to .gitignore.

Yes..let me remove it..i guess claude just sent it anyway

Base automatically changed from sq/git-e2e to main August 13, 2026 16:55
## Summary

### Why?

The gateway client only knew how to ping. Submitting a change meant hand-assembling protobuf with grpcurl, including a 40-character commit SHA copied out of a pull request — which is both tedious and easy to get subtly wrong.

### What?

`ping | land | status` subcommands. `land` takes repeated `-uri` flags, whose order is significant: several changes in one request are a stack, applied in the order given.

`-pr <url>` resolves a pull request's head commit and mints the change URI, so nobody types a SHA. It dispatches on the change URL the same way the merger and the change provider dispatch on the URI scheme, rather than assuming one provider; GitHub is implemented, and adding another is a case beside it. Reading a public repository needs no token, and a 404 without one says so rather than reporting the pull request as missing.

## Test Plan

✅ `go build ./service/submitqueue/gateway/client` and `go vet`.

✅ Manually: `land -queue q` reports the missing-change error; `land -pr https://github.com/uber/cadence/pull/1` resolved against the live GitHub API and printed `github://github.com/cadence-workflow/cadence/pull/1/65c322d4...` before failing to dial a gateway, which is the expected end of that path with no stack running.
`go build ./service/submitqueue/gateway/client` writes an 18MB executable named `client` into the current directory, and it was swept in from the repo root. The build here is Bazel-only, so nothing in the normal workflow should produce it.

Removes the binary and ignores it as root-anchored `/client`, so the pattern cannot shadow the `client/` source packages (`submitqueue/client/`, `service/submitqueue/gateway/client/`, `service/submitqueue/orchestrator/client/`).
@behinddwalls

Copy link
Copy Markdown
Collaborator Author

Removed in d39e85c3 — thanks both, and sorry for the noise.

Cause: this PR's own test plan ran go build ./service/submitqueue/gateway/client. That writes an executable named after the package directory into the current directory — run from the repo root it drops a 19,410,370-byte client there, which then got swept into the commit. The repo builds with Bazel (bazel run //service/submitqueue/gateway/client:gateway), so nothing in the normal workflow produces this file; the go build was the mistake.

Fix: binary deleted and a .gitignore entry added. One detail worth flagging — the entry is root-anchored as /client, not a bare client. A bare pattern would also match the client/ source packages (service/submitqueue/gateway/client/, service/submitqueue/orchestrator/client/, and submitqueue/client/ incoming in #570) and silently stop tracking them.

No history rewrite: main squash-merges, so the add+delete collapses to nothing and the blob never reaches main. It already exists in this PR's refs on GitHub regardless, so rewriting the branch would not remove it from there either — it would only force a restack of the branches above this one.

@behinddwalls

Copy link
Copy Markdown
Collaborator Author

Follow-up in #587, stacked on this branch: adds /server beside /client (every service/*/server package builds to a root server the same way this one built a root client), and adds make build-submitqueue-gateway-client / -server writing to bin/client and bin/server.

That gives a sanctioned way to produce a standalone binary. bin/ was already ignored but had no producer — the only reference to it anywhere was rm -rf bin/ in clean.

Keeping this PR to just the removal so it stays easy to review.

behinddwalls added a commit that referenced this pull request Aug 13, 2026
… into bin/

## Summary

### Why?

An 18MB `client` binary reached #547 and was caught only by review — nothing in CI guards against a committed binary.

`go build ./service/submitqueue/gateway/client` names its output after the package directory and writes it to the *current* directory, so run from the repo root it drops a `client` there, where a broad `git add` sweeps it up. Every `service/*/server` package produces a root `server` the same way.

The obvious fix — ignoring `/client` — is the wrong one. It would stop the file being committed but equally stop git mentioning it at all, so the mistake becomes invisible and the artifact quietly accumulates. It is also name-specific: it covers 8 of the 12 `main` packages and none of the other binaries anyone might add. Worse, an ignored path is never tracked, so it would hide the offender from the check below rather than complement it.

### What?

`tool/linter/binaryfile` fails when any tracked file is binary. It follows the existing `licenseheader` / `messageid` / `queueshard` pattern and is wired in as `make lint-binary`, so `make lint` and the CI gate already cover it. Detection is git's own heuristic — a NUL byte in the leading 8000 bytes. The repository has no tracked binaries today, so the check starts clean and the allowlist is empty.

`make build-submitqueue-gateway-client` and `make build-submitqueue-gateway-server` build with Bazel for the host platform into `bin/client` and `bin/server`, giving a sanctioned way to produce a standalone binary. `bin/` was already ignored but had no producer — the only reference to it anywhere in the Makefile, `tool/`, or `.github/` was `rm -rf bin/` in `clean`.

No `.gitignore` change is needed: `bazel-bin/`, `bin/`, and `.docker-bin/` already cover every Bazel output, and the whole point of the linter is to surface a stray binary rather than hide it.

## Test Plan

✅ `bazel test //tool/linter/binaryfile:go_default_test` passes.

✅ `make lint-binary` on a clean tree reports `All 884 tracked files are text.`

✅ Reproduced #547 end to end: `go build ./service/submitqueue/gateway/client` from the repo root drops a 17,539,170-byte `client`; `git status` shows it as `?? client` rather than hiding it; after `git add client` the linter fails, naming the file and its size and pointing at Bazel.

✅ Both build targets produce Mach-O arm64 executables at `bin/client` and `bin/server`, are idempotent over Bazel's read-only output, and leave `git status` clean.

✅ `make gazelle` generates the linter's BUILD.bazel with no further drift, and `make help` lists all three new targets.
@behinddwalls
behinddwalls added this pull request to the merge queue Aug 13, 2026
Merged via the queue into main with commit f02e9c5 Aug 13, 2026
15 checks passed
@behinddwalls
behinddwalls deployed to stack-rebase August 13, 2026 18:43 — with GitHub Actions Active
@behinddwalls
behinddwalls deleted the sq/gateway-land-cli branch August 13, 2026 18:44
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.

3 participants