Improve generation of reference docs#30
Open
adamwg wants to merge 5 commits into
Open
Conversation
Format our maturity level banners using markdown so that they look better in rendered help and generated docs. Omit the feature enablement notice in the top-level help when we generate docs, since it's not interesting on the docs website. Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates numerous CLI kong struct tag help strings and help markdown, improves docs generation/normalization (novale handling, sentinel removal, closing Hugo blocks, Markdown Notes), embeds xrd convert help with go:embed, and adds a CI job to validate generated docs with Vale. ChangesCLI Help Text and Documentation Refinement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
cmd/crossplane/xrd/generate.go (1)
53-53: ⚡ Quick winClarify what
--pathis relative toCould we keep a bit more context in the help text here?
On Line 53,Output path.is pretty terse and doesn’t tell users this is within the project APIs layout.Suggested wording
- Path string `help:"Output path." optional:""` + Path string `help:"Output path within the project's APIs directory." optional:""`As per coding guidelines: "
**/cmd/**: Review CLI commands for proper flag handling, help text, and error messages. Ensure commands follow Crossplane CLI conventions."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/xrd/generate.go` at line 53, The help text for the Path flag (struct field Path in generate.go) is too terse; update its `help` string to clearly state that the provided output path is relative to the project root and should point into the APIs layout (e.g., under the project's api/ or apis/ directory), e.g. "Output path relative to project root (destination within project APIs layout, e.g. api/ or apis/)." Ensure you only change the `help:"..."` value for the Path field.cmd/crossplane/function/help/generate.md (1)
2-3: 💤 Low valueConsider whether "idempotently" is clear for all users.
The term "idempotently" is technically precise but may not be familiar to all CLI users. Since this help text falls under
cmd/**, the coding guidelines emphasize user-friendliness and avoiding internal technical details.Would simpler phrasing like "adds the new function to the end of a Composition's pipeline (if not already present)" or "safely adds the new function" be clearer while still conveying the behavior?
That said, if "idempotently" is intentional and aligns with Crossplane terminology conventions, it's fine to keep—just wanted to check!
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/function/help/generate.md` around lines 2 - 3, The help text uses the term "idempotently" which may be unclear to some users; replace that word in the generate help text where it says "idempotently adds the new function to end of a Composition's pipeline" with clearer wording such as "adds the new function to the end of a Composition's pipeline (if not already present)" or "safely adds the new function to the end of a Composition's pipeline" so the behavior is explicit and user-friendly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/crossplane/docs.go`:
- Around line 237-242: The code currently closes the "hint" shortcode at EOF
using the bq flag but forgets to close an open table shortcode; update the EOF
handling in the same function that builds sb (where bq is used) to also check
the table-open flag (e.g., bt) and, if true, append the table close token "{{<
/table >}}\n" to sb before returning strings.TrimSpace(sb.String()), so any
trailing table row is properly terminated.
In `@cmd/crossplane/function/help/generate.md`:
- Line 3: Edit the sentence "adds the new function to end of a Composition's
pipeline when given a" to insert the missing article so it reads "adds the new
function to the end of a Composition's pipeline when given a" to correct the
grammar.
In `@cmd/crossplane/project/help/build.md`:
- Line 8: The Markdown text contains an extra space inside the inline code
backticks for the build command; locate the literal token "`build `" in the help
text (the sentence starting "The `build `command constructs...") and remove the
trailing space so it reads "`build`" (i.e., replace "`build `" with "`build`")
to fix the formatting.
- Line 5: Fix the typo in the project help text: replace the misspelled word
"packges" with "packages" in the sentence that mentions the `project push`
command (the line containing "The `project push` command can consume packges
from the"). Ensure only the spelling is corrected and punctuation/formatting
remains unchanged.
In `@cmd/crossplane/project/help/stop.md`:
- Line 8: Replace the incorrect command references 'up project run' and 'up
project stop' in the stop.md content with the correct commands 'crossplane
project run' and 'crossplane project stop' so the README/documentation
accurately reflects the CLI; locate the offending strings (e.g., "passed
`--registry-dir` to `up project run`, pass it to `up project stop` as") and
update them to use "crossplane project run" and "crossplane project stop".
In `@cmd/crossplane/trace/help/trace.md`:
- Around line 35-37: Fix the typo in the help text: change the phrase
"graph.Pipe to `dot` to" so there is a space after the period (i.e., "graph.
Pipe to `dot` to") in the string containing "Use `--output=dot` to print a
textual [Graphviz dot](https://graphviz.org/docs/layouts/dot/) graph.Pipe to
`dot` to render an image"; update that literal so the sentence separation is
correct.
---
Nitpick comments:
In `@cmd/crossplane/function/help/generate.md`:
- Around line 2-3: The help text uses the term "idempotently" which may be
unclear to some users; replace that word in the generate help text where it says
"idempotently adds the new function to end of a Composition's pipeline" with
clearer wording such as "adds the new function to the end of a Composition's
pipeline (if not already present)" or "safely adds the new function to the end
of a Composition's pipeline" so the behavior is explicit and user-friendly.
In `@cmd/crossplane/xrd/generate.go`:
- Line 53: The help text for the Path flag (struct field Path in generate.go) is
too terse; update its `help` string to clearly state that the provided output
path is relative to the project root and should point into the APIs layout
(e.g., under the project's api/ or apis/ directory), e.g. "Output path relative
to project root (destination within project APIs layout, e.g. api/ or apis/)."
Ensure you only change the `help:"..."` value for the Path field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 397ae2e7-ea18-4121-bbb4-d3881e7e4f95
⛔ Files ignored due to path filters (1)
cmd/crossplane/docs-templates/command-reference.md.tmplis excluded by none and included by none
📒 Files selected for processing (48)
cmd/crossplane/composition/generate.gocmd/crossplane/composition/help/generate.mdcmd/crossplane/config/config.gocmd/crossplane/config/help/config.mdcmd/crossplane/config/set.gocmd/crossplane/convert/compositionenvironment/cmd.gocmd/crossplane/convert/compositionenvironment/help/composition-environment.mdcmd/crossplane/convert/help/convert.mdcmd/crossplane/dependency/add.gocmd/crossplane/dependency/help/add.mdcmd/crossplane/dependency/help/update-cache.mdcmd/crossplane/docs.gocmd/crossplane/function/help/generate.mdcmd/crossplane/main.gocmd/crossplane/project/help/build.mdcmd/crossplane/project/help/init.mdcmd/crossplane/project/help/push.mdcmd/crossplane/project/help/run.mdcmd/crossplane/project/help/stop.mdcmd/crossplane/project/push.gocmd/crossplane/project/run.gocmd/crossplane/render/engine.gocmd/crossplane/render/op/cmd.gocmd/crossplane/render/op/help/render.mdcmd/crossplane/render/xr/cmd.gocmd/crossplane/render/xr/help/render.mdcmd/crossplane/top/help/top.mdcmd/crossplane/trace/help/trace.mdcmd/crossplane/trace/trace.gocmd/crossplane/validate/cmd.gocmd/crossplane/validate/help/validate.mdcmd/crossplane/xpkg/batch.gocmd/crossplane/xpkg/build.gocmd/crossplane/xpkg/extract.gocmd/crossplane/xpkg/help/build.mdcmd/crossplane/xpkg/help/init.mdcmd/crossplane/xpkg/help/install.mdcmd/crossplane/xpkg/help/push.mdcmd/crossplane/xpkg/help/update.mdcmd/crossplane/xpkg/help/xpkg.mdcmd/crossplane/xpkg/init.gocmd/crossplane/xpkg/install.gocmd/crossplane/xpkg/update.gocmd/crossplane/xrd/convert.gocmd/crossplane/xrd/generate.gocmd/crossplane/xrd/help/convert.mdcmd/crossplane/xrd/help/generate.mdinternal/maturity/maturity.go
The crossplane/docs repository uses the vale tool to lint prose. It's a little overzealous sometimes, but overall helpful, and currently complains about a ton of issues in our help text. Fix as many issues as possible. Use a special spell check (to be introduced in the docs repo) to avoid complaints about lower-case `crossplane`, since it's the command name. The remaining issues we can't fix are: - The abbreviation `config` in the `crossplane config` command. - The abbreviation `k8s` in the flag help for `crossplane dependency add`. - The abbreviation `admin` in `cluster-admin` in the help for `crossplane project run`. Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
Fix a few spelling/grammar/typo issues caught by CodeRabbit, and close tables if the last line of help is a table row. Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
We have a few commands whose basic (non-detail) help causes vale errors. Specifically: * `config` is a forbidden word, so `crossplane config` triggers a warning. * `k8s` is forbidden confuses a few checks, and we use it in the help for `crossplane dependency add`. * `admin` is a forbidden word, but it's necessary to name the `cluster-admin` role in the help for `crossplane project run`. Add the ability to tag commands with `novale:"..."`, causing specific vale rules to be ignored for that command's section of the help. Introduce these on the commands named above. Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.github/workflows/ci.yml (1)
203-206: ⚡ Quick winConsider adding checksum validation for the Vale download.
The workflow downloads and executes a Vale binary without verifying its integrity. While the risk is relatively low with a pinned version over HTTPS, adding SHA256 checksum validation would follow security best practices and protect against potential supply-chain attacks or MITM scenarios.
🔐 Proposed enhancement
You can add checksum validation like this:
- name: Install Vale env: VALE_VERSION: "3.14.2" + VALE_SHA256: "a1b2c3d4e5f6..." # Replace with actual checksum from release page run: | - wget https://github.com/errata-ai/vale/releases/download/v${{env.VALE_VERSION}}/vale_${{env.VALE_VERSION}}_Linux_64-bit.tar.gz && tar -xvzf vale_${{env.VALE_VERSION}}_Linux_64-bit.tar.gz + wget https://github.com/errata-ai/vale/releases/download/v${{env.VALE_VERSION}}/vale_${{env.VALE_VERSION}}_Linux_64-bit.tar.gz + echo "${{env.VALE_SHA256}} vale_${{env.VALE_VERSION}}_Linux_64-bit.tar.gz" | sha256sum -c + tar -xvzf vale_${{env.VALE_VERSION}}_Linux_64-bit.tar.gzThe checksum can be found on the Vale releases page.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 203 - 206, The "Install Vale" workflow step currently downloads and extracts the Vale tarball without verifying integrity; add a SHA256 checksum validation using the VALE_VERSION variable before extracting: download or define the expected checksum for vale_${{env.VALE_VERSION}}_Linux_64-bit.tar.gz, verify the downloaded file with sha256sum (or similar) and fail the job on mismatch, then only tar -xvzf after successful verification; update the "Install Vale" step to perform the checksum fetch/verification and ensure the step exits non-zero on verification failure.cmd/crossplane/docs.go (1)
131-133: ⚡ Quick winConsider trimming whitespace from individual novale rules.
If a developer writes
novale:"rule1, rule2"(with a space after the comma), the split will preserve that leading space in "rule2". This could cause the generated vale comment to have extra whitespace.🧹 Proposed enhancement
if n.Tag != nil && n.Tag.Get("novale") != "" { - dc.NoVale = strings.Split(n.Tag.Get("novale"), ",") + parts := strings.Split(n.Tag.Get("novale"), ",") + for i := range parts { + parts[i] = strings.TrimSpace(parts[i]) + } + dc.NoVale = parts }Or more concisely using a helper:
if n.Tag != nil && n.Tag.Get("novale") != "" { - dc.NoVale = strings.Split(n.Tag.Get("novale"), ",") + dc.NoVale = slices.Map(strings.Split(n.Tag.Get("novale"), ","), strings.TrimSpace) }(Note:
slices.Maprequires Go 1.24+; if not available, use the loop version)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/docs.go` around lines 131 - 133, The current assignment to dc.NoVale uses strings.Split(n.Tag.Get("novale"), ",") which preserves any surrounding whitespace; update the logic in cmd/crossplane/docs.go where n.Tag.Get("novale") is read so each entry is trimmed (e.g., map each element with strings.TrimSpace or use slices.Map + strings.TrimSpace) before assigning to dc.NoVale; ensure empty strings are preserved/handled consistently as the original behavior requires.cmd/crossplane/trace/trace.go (1)
80-80: 💤 Low valueCapitalization consistency check.
The help text for
Concurrencyis "load concurrency" (lowercase, no period), while all other flag help texts in this struct start with a capital letter and end with a period. Was the lowercase intentional (perhaps to address a specific Vale rule), or would "Load concurrency." be more consistent with the rest of the help text?Thanks for improving the help text clarity throughout this command! 🎉
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/trace/trace.go` at line 80, The help text for the Concurrency flag is inconsistent with other flags (it reads "load concurrency" lowercase and no period); update the struct tag on Concurrency to use sentence-style capitalization and punctuation (e.g., "Load concurrency.") so it matches the rest of the flags' help texts; locate the Concurrency field in the trace.go struct and change the help tag value accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 188-191: The checkout step that uses
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd to fetch the
crossplane/docs repository should include persist-credentials: false to avoid
persisting the GITHUB_TOKEN to the checked-out repository; update the step that
currently has "uses: actions/checkout@de0fac2e..." and "with: repository:
crossplane/docs" to add a "persist-credentials: false" field under with so the
token is not stored in the local git config during that checkout.
- Line 210: Fix the unmatched double-quote in the Vale invocation: the flag
--config=docs/utils/vale/.vale.ini" has an extra trailing quote causing a shell
syntax error; remove the stray quote (or alternatively add a matching opening
quote so both the --config value and the target path are properly quoted) in the
./vale ... | tee ... command so the --config argument is valid and the
docs/content/master/cli/command-reference.md path is passed correctly.
In `@cmd/crossplane/xrd/convert.go`:
- Line 43: The change made InputFile (the struct field tagged as InputFile in
the convert command) to default to "-" which causes the command to block on
stdin when no arg is provided; add a unit/integration test that invokes the
convert command with no args (indirectly exercising the InputFile default) and
asserts the expected behavior (e.g., reads from provided stdin data and exits)
to cover the no-arg case, and update the command help text in
cmd/crossplane/xrd/help/convert.md to explicitly state that stdin is the default
when no input file is supplied so automation authors are not surprised.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 203-206: The "Install Vale" workflow step currently downloads and
extracts the Vale tarball without verifying integrity; add a SHA256 checksum
validation using the VALE_VERSION variable before extracting: download or define
the expected checksum for vale_${{env.VALE_VERSION}}_Linux_64-bit.tar.gz, verify
the downloaded file with sha256sum (or similar) and fail the job on mismatch,
then only tar -xvzf after successful verification; update the "Install Vale"
step to perform the checksum fetch/verification and ensure the step exits
non-zero on verification failure.
In `@cmd/crossplane/docs.go`:
- Around line 131-133: The current assignment to dc.NoVale uses
strings.Split(n.Tag.Get("novale"), ",") which preserves any surrounding
whitespace; update the logic in cmd/crossplane/docs.go where n.Tag.Get("novale")
is read so each entry is trimmed (e.g., map each element with strings.TrimSpace
or use slices.Map + strings.TrimSpace) before assigning to dc.NoVale; ensure
empty strings are preserved/handled consistently as the original behavior
requires.
In `@cmd/crossplane/trace/trace.go`:
- Line 80: The help text for the Concurrency flag is inconsistent with other
flags (it reads "load concurrency" lowercase and no period); update the struct
tag on Concurrency to use sentence-style capitalization and punctuation (e.g.,
"Load concurrency.") so it matches the rest of the flags' help texts; locate the
Concurrency field in the trace.go struct and change the help tag value
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4fc03f46-a943-42cb-ba69-0ba00292806f
⛔ Files ignored due to path filters (1)
cmd/crossplane/docs-templates/command-reference.md.tmplis excluded by none and included by none
📒 Files selected for processing (50)
.github/workflows/ci.ymlcmd/crossplane/composition/generate.gocmd/crossplane/composition/help/generate.mdcmd/crossplane/config/config.gocmd/crossplane/config/help/config.mdcmd/crossplane/config/set.gocmd/crossplane/convert/compositionenvironment/cmd.gocmd/crossplane/convert/compositionenvironment/help/composition-environment.mdcmd/crossplane/convert/help/convert.mdcmd/crossplane/dependency/add.gocmd/crossplane/dependency/dependency.gocmd/crossplane/dependency/help/add.mdcmd/crossplane/dependency/help/update-cache.mdcmd/crossplane/docs.gocmd/crossplane/function/help/generate.mdcmd/crossplane/main.gocmd/crossplane/project/help/build.mdcmd/crossplane/project/help/init.mdcmd/crossplane/project/help/push.mdcmd/crossplane/project/help/run.mdcmd/crossplane/project/help/stop.mdcmd/crossplane/project/project.gocmd/crossplane/project/push.gocmd/crossplane/project/run.gocmd/crossplane/render/engine.gocmd/crossplane/render/op/cmd.gocmd/crossplane/render/op/help/render.mdcmd/crossplane/render/xr/cmd.gocmd/crossplane/render/xr/help/render.mdcmd/crossplane/top/help/top.mdcmd/crossplane/trace/help/trace.mdcmd/crossplane/trace/trace.gocmd/crossplane/validate/cmd.gocmd/crossplane/validate/help/validate.mdcmd/crossplane/xpkg/batch.gocmd/crossplane/xpkg/build.gocmd/crossplane/xpkg/extract.gocmd/crossplane/xpkg/help/build.mdcmd/crossplane/xpkg/help/init.mdcmd/crossplane/xpkg/help/install.mdcmd/crossplane/xpkg/help/push.mdcmd/crossplane/xpkg/help/update.mdcmd/crossplane/xpkg/help/xpkg.mdcmd/crossplane/xpkg/init.gocmd/crossplane/xpkg/install.gocmd/crossplane/xpkg/update.gocmd/crossplane/xrd/convert.gocmd/crossplane/xrd/generate.gocmd/crossplane/xrd/help/convert.mdcmd/crossplane/xrd/help/generate.md
✅ Files skipped from review due to trivial changes (38)
- cmd/crossplane/convert/help/convert.md
- cmd/crossplane/function/help/generate.md
- cmd/crossplane/convert/compositionenvironment/help/composition-environment.md
- cmd/crossplane/project/help/stop.md
- cmd/crossplane/convert/compositionenvironment/cmd.go
- cmd/crossplane/project/help/init.md
- cmd/crossplane/config/set.go
- cmd/crossplane/dependency/help/add.md
- cmd/crossplane/top/help/top.md
- cmd/crossplane/composition/generate.go
- cmd/crossplane/xrd/help/generate.md
- cmd/crossplane/xpkg/help/push.md
- cmd/crossplane/validate/cmd.go
- cmd/crossplane/render/xr/cmd.go
- cmd/crossplane/xpkg/build.go
- cmd/crossplane/dependency/add.go
- cmd/crossplane/xpkg/help/install.md
- cmd/crossplane/xpkg/extract.go
- cmd/crossplane/composition/help/generate.md
- cmd/crossplane/trace/help/trace.md
- cmd/crossplane/main.go
- cmd/crossplane/dependency/help/update-cache.md
- cmd/crossplane/project/help/run.md
- cmd/crossplane/xpkg/help/update.md
- cmd/crossplane/xpkg/batch.go
- cmd/crossplane/project/push.go
- cmd/crossplane/project/help/push.md
- cmd/crossplane/render/op/cmd.go
- cmd/crossplane/config/help/config.md
- cmd/crossplane/xpkg/help/xpkg.md
- cmd/crossplane/render/engine.go
- cmd/crossplane/xpkg/help/build.md
- cmd/crossplane/xrd/generate.go
- cmd/crossplane/xrd/help/convert.md
- cmd/crossplane/xpkg/install.go
- cmd/crossplane/project/help/build.md
- cmd/crossplane/validate/help/validate.md
- cmd/crossplane/xpkg/help/init.md
adamwg
force-pushed
the
awg/improve-docs
branch
2 times, most recently
from
8e5f659 to
7b7abcc
Compare
Member
Author
|
The new |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 214: The pipeline that runs Vale ("./vale
--config=\"./docs/utils/vale/.vale.ini\"
./docs/content/master/cli/command-reference.md | tee /dev/stderr | wc -l | {
read wc; test $wc -eq 1; }") should be replaced to rely on Vale's exit status
instead of counting output lines; update the CI step that invokes "./vale
--config=\"./docs/utils/vale/.vale.ini\"
./docs/content/master/cli/command-reference.md" so it propagates Vale's exit
code (e.g., run Vale directly and fail the job on non-zero exit, or append "||
exit 1") and remove the tee/wc/read/test pipeline so CI fails when Vale returns
non-zero.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bacb62d9-0397-4dde-9c6e-8fdbe2f22589
📒 Files selected for processing (1)
.github/workflows/ci.yml
After building the CLI, check out the docs repo, use our freshly built binary to generate the CLI reference documentation, and validate it using the docs repo's vale config. This will ensure our generated docs always pass CI when put up for review in the docs repo. Fixes crossplane#32 Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 207-210: The workflow currently downloads the Vale tarball with
wget and extracts it with tar without integrity checks; add a pinned VALE_SHA256
variable and verify the downloaded archive before extraction by computing and
validating its sha256 sum (e.g., download $VALE_URL to vale.tar.gz, then run
sha256sum -c or echo "$VALE_SHA256 vale.tar.gz" | sha256sum -c -) and fail the
step if verification fails; update the step that runs wget/tar to reference
VALE_SHA256 and perform the sha256 check prior to running tar to ensure the
archive matches the expected checksum.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e1e6667-554b-4b1f-988b-8b5064aaea2e
📒 Files selected for processing (1)
.github/workflows/ci.yml
Comment on lines
+207
to
+210
| - name: Install Vale | ||
| env: | ||
| VALE_VERSION: "3.14.2" | ||
| run: wget https://github.com/errata-ai/vale/releases/download/v${{env.VALE_VERSION}}/vale_${{env.VALE_VERSION}}_Linux_64-bit.tar.gz && tar -xvzf vale_${{env.VALE_VERSION}}_Linux_64-bit.tar.gz |
There was a problem hiding this comment.
Verify Vale download is checksum-validated (Line 210)
The workflow downloads the Vale tarball with wget and immediately extracts it with tar without any integrity check. Add checksum/signature verification for the exact archive before extraction—either by validating against Vale’s published checksums or by pinning an expected VALE_SHA256 in the workflow and running sha256sum -c before tar.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 207 - 210, The workflow currently
downloads the Vale tarball with wget and extracts it with tar without integrity
checks; add a pinned VALE_SHA256 variable and verify the downloaded archive
before extraction by computing and validating its sha256 sum (e.g., download
$VALE_URL to vale.tar.gz, then run sha256sum -c or echo "$VALE_SHA256
vale.tar.gz" | sha256sum -c -) and fail the step if verification fails; update
the step that runs wget/tar to reference VALE_SHA256 and perform the sha256
check prior to running tar to ensure the archive matches the expected checksum.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
The crossplane/docs repository uses the vale tool to lint prose. It's a overzealous sometimes, but overall helpful, and currently complains about a ton of issues in our help text.
Fix all the issues, using a special spell check (to be introduced in the docs repo) to avoid complaints about lower-case
crossplane, since it's the command name, and a newnovalekong tag to disable specific checks where we can't fix them otherwise (e.g., when the command name is a forbidden word).To address some of the vale issues, and make the output look better, format our maturity level banners using markdown. Omit the feature enablement notice in the top-level help when we generate docs, since it's not interesting on the docs website.
While we're here, introduce a CI check that generates the docs and runs the docs repo's vale check on them. This will probably fail until crossplane/docs#1097 is merged since it makes a necessary update to the vale configuration.
Fixes #32
I have:
./nix.sh flake checkto ensure this PR is ready for review.- [ ] Added or updated unit tests.- [ ] Linked a PR or a docs tracking issue to document this change.backport release-x.ylabels to auto-backport this PR.