bazel: build http docs w bazel #68834
Conversation
|
This change is |
rickystewart
force-pushed
the
http_docs
branch
2 times, most recently
from
1630fd1 to
cb74bb7
Compare
rickystewart
force-pushed
the
http_docs
branch
from
cb74bb7 to
02660a0
Compare
There was a problem hiding this comment.
In overall it looks great. Just a few nits.
Reviewed 7 of 8 files at r2, 13 of 13 files at r3, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @rickystewart)
pkg/cmd/docgen/http.go, line 88 at r3 (raw file):
}() var args []string if len(protocPath) == 0 {
This check made me think twice. :) Can you use if protocPath == "" instead to highlight we are comparing a string and not a slice (len() made me think it's a slice).
pkg/cmd/docgen/http.go, line 95 at r3 (raw file):
fmt.Sprintf("--doc_opt=%s,http.json", jsonTmpl), fmt.Sprintf("--plugin=protoc-gen-doc=%s", genDocPath)) args = append(args, strings.Fields(protocFlags)...)
It'd be safer to use something like https://pkg.go.dev/github.com/google/shlex, but probably not a big deal now.
pkg/cmd/docgen/http.go, line 98 at r3 (raw file):
// Generate the JSON file. executable := protocPath if len(protocPath) == 0 {
And same here, can you if protocPath == "" instead?
We generate this in a different way than the `Makefile` does -- we take all the proto descriptor sets (that we need to generate anyway as part of the build) and pass them in to `protoc` rather than passing a bunch of include paths, which wouldn't work terribly well in the sandbox. Also teach CI and `dev` to start building these docs. I deleted a stray comment in `pkg/server/serverpb/admin.proto` that was causing a diff in the generated file. I also had to patch `@com_github_pseudomuto_protoc_gen_doc` to quash a compiler error (I just deleted a dependency that isn't actually necessary). Closes cockroachdb#65814. Release note: None
rickystewart
force-pushed
the
http_docs
branch
from
02660a0 to
29a8388
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/docgen/http.go, line 88 at r3 (raw file):
Previously, rail (Rail Aliiev) wrote…
This check made me think twice. :) Can you use
if protocPath == ""instead to highlight we are comparing a string and not a slice (len()made me think it's a slice).
Done.
pkg/cmd/docgen/http.go, line 98 at r3 (raw file):
Previously, rail (Rail Aliiev) wrote…
And same here, can you
if protocPath == ""instead?
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:
|
bors r=rail |
|
Build succeeded: |
We generate this in a different way than the
Makefiledoes -- we takeall the proto descriptor sets (that we need to generate anyway as part
of the build) and pass them in to
protocrather than passing a bunchof include paths, which wouldn't work terribly well in the sandbox.
Also teach CI and
devto start building these docs.I deleted a stray comment in
pkg/server/serverpb/admin.protothat wascausing a diff in the generated file. I also had to patch
@com_github_pseudomuto_protoc_gen_docto quash a compiler error (Ijust deleted a dependency that isn't actually necessary).
Closes #65814.
Release note: None