Conversation
Signed-off-by: lance6716 <lance6716@gmail.com>
|
|
There was a problem hiding this comment.
Pull request overview
This PR introduces deterministic .proto formatting by standardizing on buf format, integrating it into local workflows (make) and CI, and applying the formatter across the existing protobuf definitions.
Changes:
- Add a
scripts/proto_format.shhelper that installs a pinnedbufbinary and runs formatting in check/write modes. - Wire proto-format checks into
make check, add dedicatedmake proto-fmt/make proto-fmt-checktargets, and add a CI job to enforce formatting. - Reformat multiple
.protofiles (andinclude/rustproto.proto) to matchbuf formatoutput.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/proto_format.sh | New buf installer + formatter wrapper used by make/CI |
| scripts/check.sh | Run proto formatting verification as part of existing checks |
| Makefile | Add proto-fmt / proto-fmt-check targets and update phony list |
| .github/workflows/unit-test.yml | Add CI job to enforce proto formatting |
| README.md | Document proto formatting workflow and make targets |
| proto/trace.proto | Formatting-only changes via buf |
| proto/topsql_agent.proto | Formatting-only changes via buf |
| proto/tici/indexer.proto | Formatting-only changes via buf |
| proto/select.proto | Formatting-only changes via buf |
| proto/schema.proto | Formatting-only changes via buf |
| proto/resourcetag.proto | Formatting-only changes via buf |
| proto/metadata.proto | Formatting-only changes via buf |
| proto/expression.proto | Formatting-only changes via buf |
| proto/explain.proto | Formatting-only changes via buf |
| proto/executor.proto | Formatting-only changes via buf (incl. import/whitespace normalization) |
| proto/checksum.proto | Formatting-only changes via buf |
| proto/analyze.proto | Formatting-only changes via buf |
| include/rustproto.proto | Formatting-only changes via buf |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| url="https://github.com/bufbuild/buf/releases/download/v${BUF_VERSION}/buf-${os}-${arch}" | ||
| echo "Installing buf v${BUF_VERSION} to ${BUF_BIN}" >&2 | ||
| curl -sSfL "${url}" -o "${BUF_BIN}" | ||
| chmod +x "${BUF_BIN}" |
There was a problem hiding this comment.
install_buf downloads and executes a prebuilt binary from GitHub without any integrity verification. This is a supply-chain risk (MITM/compromised release asset) and makes CI less trustworthy. Consider downloading the corresponding checksum file (or using a signed release mechanism) and verifying the SHA256 before chmod +x / execution, failing the script if the verification does not match.
| install_buf() { | ||
| if [ -x "${BUF_BIN}" ]; then | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
install_buf only checks whether ${BUF_BIN} is executable; if a previously-downloaded bin/buf exists, it will be reused even when BUF_VERSION changes (or if the file is corrupted). To keep formatting deterministic, consider checking ${BUF_BIN} --version against BUF_VERSION (and re-downloading on mismatch), and downloading to a temp file then atomically moving it into place to avoid leaving a partial binary if curl is interrupted.
| echo "Installing buf v${BUF_VERSION} to ${BUF_BIN}" >&2 | ||
| curl -sSfL "${url}" -o "${BUF_BIN}" | ||
| chmod +x "${BUF_BIN}" |
There was a problem hiding this comment.
The curl -sSfL download has no retries/timeouts, which can make CI flaky on transient network issues. Consider adding --retry (and optionally --retry-all-errors), plus reasonable connect/overall timeouts so failures are fast and actionable.
1 participant
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
What is changed and how it works?
Proposal: xxx
What's Changed:
How it Works:
Related changes
pingcap/docs/pingcap/docs-cn: