fix(taskfiles): Output unexpected errors in checksum tasks; Call gtar on macos; Add unit tests for checksum tasks (fixes #67). #81
Conversation
WalkthroughAdds macOS prerequisites to README; refactors checksum tasks to use an OS-aware tar (gtar on macOS), stream tar → md5sum, and introduce TMP error logs plus a FAIL parameter to control fail-soft/fail-fast behavior; validate computes to a temp file and conditionally fails or cleans up; adds checksum test suite and integrates it into tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Compute as checksum:compute
participant TAR as tar/gtar
participant MD5 as md5sum
participant FS as Filesystem
participant ERR as TMP_ERR_LOG
Caller->>Compute: invoke(INCLUDE_PATTERNS, EXCLUDE_PATTERNS, CHECKSUM_FILE, FAIL)
Compute->>Compute: select TAR (darwin → gtar, else tar)
Compute->>TAR: create archive (stdout) with include/exclude
TAR-->>ERR: stderr → TMP_ERR_LOG
TAR-->>MD5: stdout (archive stream)
MD5-->>FS: write CHECKSUM_FILE
alt tar or md5 failure
Compute->>ERR: read errors
alt FAIL=true
Compute-->>Caller: exit 1 with error details
else FAIL=false
Compute-->>Caller: exit 0 (non-fatal, checksum not published)
end
else success
Compute-->>Caller: exit 0
end
sequenceDiagram
autonumber
participant Caller
participant Validate as checksum:validate
participant ComputeTmp as checksum:compute (tmp)
participant FS as Filesystem
participant ERR as TMP_ERR_LOG
Caller->>Validate: invoke(INCLUDE_PATTERNS, EXCLUDE_PATTERNS, CHECKSUM_FILE, FAIL)
Validate->>FS: verify INCLUDE paths exist (log missing to TMP_ERR_LOG)
Validate->>ComputeTmp: compute TMP_CHECKSUM_FILE
ComputeTmp-->>Validate: exit code
Validate->>FS: compare TMP_CHECKSUM_FILE vs CHECKSUM_FILE
alt any failure or mismatch
alt FAIL=true
Validate->>ERR: aggregate logs
Validate-->>Caller: exit 1 with error details
else FAIL=false
Validate->>FS: remove CHECKSUM_FILE (allow rerun)
Validate-->>Caller: exit 0
end
else success
Validate-->>Caller: exit 0
end
Validate->>FS: cleanup TMP files (defer)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (13)📓 Common learnings📚 Learning: 2024-11-15T02:34:27.126ZApplied to files:
📚 Learning: 2024-10-28T19:02:02.131ZApplied to files:
📚 Learning: 2025-10-13T19:09:44.404ZApplied to files:
📚 Learning: 2025-08-26T16:37:16.741ZApplied to files:
📚 Learning: 2025-08-26T16:37:16.741ZApplied to files:
📚 Learning: 2025-10-14T12:14:41.400ZApplied to files:
📚 Learning: 2025-02-14T11:49:28.430ZApplied to files:
📚 Learning: 2024-10-30T22:08:49.844ZApplied to files:
📚 Learning: 2025-06-08T17:04:50.095ZApplied to files:
📚 Learning: 2025-08-24T14:25:42.436ZApplied to files:
📚 Learning: 2025-06-08T17:55:11.362ZApplied to files:
📚 Learning: 2025-09-24T20:49:45.683ZApplied to files:
🔇 Additional comments (1)
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
taskfiles/checksum/tests.yaml (1)
141-149: Status inputs: consider using directory contents as sources for better cache invalidationCurrently
create-dir-with-checksumusessources: ["{{.TASKFILE}}"], so changes to files under DIR won’t trigger a rebuild via Task’s up-to-date checks; you rely on checksum:validate instead. That’s fine for this test, but if you want Task’s native skipping to reflect content changes, consider adding the directory as a source (globbed).Optional example:
- sources: ["{{.TASKFILE}}"] + sources: + - "{{.DIR}}/**" + - "{{.TASKFILE}}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
README.md(2 hunks)exports/taskfiles/utils/checksum.yaml(3 hunks)taskfiles/checksum/tests.yaml(1 hunks)taskfiles/tests.yaml(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~17-~17: The operating system from Apple is written “macOS”.
Context: ...es that are not always pre-installed on MacOS. You may need to install the following ...
(MAC_OS)
🔇 Additional comments (2)
taskfiles/tests.yaml (1)
13-16: LGTM: checksum tests are now part of the default suiteIncluding checksum tests in tasks.all makes the new behaviour exercised continuously. Good move, and the commented boost line keeps context.
taskfiles/checksum/tests.yaml (1)
64-64: Avoid Bash-only process substitution and make mtime checks cross-platform
cmp -s '…' <(date -r …)requires Bash and will fail under /bin/sh (dash) and on macOS (date -r semantics differ).- Use stat with an OS guard and plain files for comparison.
Apply:
- - "date -r '{{.CHECKSUM_FILE}}' > '{{.CHECKSUM_MOD_TS}}'" + - |- + if [ "$(uname)" = "Darwin" ]; then + stat -f %m '{{.CHECKSUM_FILE}}' > '{{.CHECKSUM_MOD_TS}}' + else + stat -c %Y '{{.CHECKSUM_FILE}}' > '{{.CHECKSUM_MOD_TS}}' + fi @@ - - "cmp -s '{{.CHECKSUM_MOD_TS}}' <(date -r '{{.CHECKSUM_FILE}}')" + - |- + if [ "$(uname)" = "Darwin" ]; then + stat -f %m '{{.CHECKSUM_FILE}}' > '{{.CHECKSUM_FILE}}-mod-ts.new.txt' + else + stat -c %Y '{{.CHECKSUM_FILE}}' > '{{.CHECKSUM_FILE}}-mod-ts.new.txt' + fi + cmp -s '{{.CHECKSUM_MOD_TS}}' '{{.CHECKSUM_FILE}}-mod-ts.new.txt'Also applies to: 73-73
⛔ Skipped due to learnings
Learnt from: davidlion PR: y-scope/yscope-dev-utils#70 File: taskfiles/remote/tests.yaml:62-72 Timestamp: 2025-07-12T05:52:11.439Z Learning: The `date -r <file>` command is supported on both Linux and macOS to get file modification times, making it portable across these platforms.Learnt from: AVMatthews PR: y-scope/yscope-dev-utils#13 File: taskfiles/utils.yml:149-149 Timestamp: 2024-10-28T19:02:02.131Z Learning: In `taskfiles/utils.yml`, avoid using GNU-specific options like `-r` and `--no-run-if-empty` with `xargs` for better portability; instead, use `find`'s `-exec` option.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
exports/taskfiles/utils/checksum.yaml (1)
30-37: Globbing with spaces may mis-split; verify this is acceptablePatterns are intentionally unquoted so the shell expands them, but this will split on whitespace in file names. If your repo allows spaces, you’ll need a different enumeration strategy (e.g., tar -T with a nul list). If spaces are out of scope, consider documenting that.
Also applies to: 55-56
♻️ Duplicate comments (5)
README.md (1)
15-22: macOS prerequisites: add PATH instructions for Homebrew “gnubin” and clarify tool namesReaders will otherwise miss that md5sum and gtar won’t exist unless gnubin is prepended to PATH on macOS. Also, prefer “Homebrew packages” and remove the escaped colons; use brief “why” notes.
-### macOS - -The exported tasks use GNU utilities that are not always pre-installed on macOS. You may need to -install the following brew packages and add their executables to your PATH: - -* [coreutils]\: `md5sum` -* [gnu-tar]\: `gtar` +### macOS + +The exported tasks use GNU utilities that are not always pre-installed on macOS. Install the +following Homebrew packages and add their “gnubin” shims to your PATH so `md5sum` and `gtar` +are available under the expected names: + +* [coreutils] (provides `md5sum` and friends) +* [gnu-tar] (provides `gtar`) + +Add to your shell profile (adjust for Intel vs Apple Silicon): + +```sh +export PATH="$(brew --prefix coreutils)/libexec/gnubin:$(brew --prefix gnu-tar)/libexec/gnubin:$PATH" +```exports/taskfiles/utils/checksum.yaml (4)
19-21: Nil-safe handling of EXCLUDE_PATTERNSRelying on
ref: "default (list) .EXCLUDE_PATTERNS"is brittle; use a nil-safe default at the callsite so ranging a missing var doesn’t error.- EXCLUDE_PATTERNS: - ref: "default (list) .EXCLUDE_PATTERNS" + EXCLUDE_PATTERNS: + ref: ".EXCLUDE_PATTERNS" @@ - {{- range .EXCLUDE_PATTERNS}} + {{- range (default (list) .EXCLUDE_PATTERNS) }} --exclude="{{.}}" \ {{- end}}If you prefer keeping the default at definition time, consider a shell-evaluated var:
+ EXCLUDE_PATTERNS: + sh: 'echo ""' # default to empty list; still wrap with default(list) when rangingAlso applies to: 51-53
38-66: Pipeline can produce bogus checksums if tar fails; avoid relying on pipeline exit statusBecause the pipeline status is that of
md5sum(no pipefail), tar errors won’t cause the if-guard to fail, and you may hash empty/partial input. Write tar output to a temp file, check tar’s exit, then hash and atomically move the checksum.- - defer: "rm -f '{{.TMP_ERR_LOG}}'" + - defer: "rm -f '{{.TMP_ERR_LOG}}' '{{.TMP_TAR_FILE}}' '{{.TMP_CHECKSUM_FILE}}'" - |- - if ! \ - {{.TAR}} \ - --create \ - --file - \ + if ! \ + {{.TAR}} \ + --create \ + --file "{{.TMP_TAR_FILE}}" \ --group 0 \ --mtime "UTC 1970-01-01" \ --numeric-owner \ --owner 0 \ --sort name \ --no-anchored \ --wildcards \ - {{- range .EXCLUDE_PATTERNS}} + {{- range (default (list) .EXCLUDE_PATTERNS) }} --exclude="{{.}}" \ {{- end}} {{- range .INCLUDE_PATTERNS}} {{.}} \ {{- end}} - 2> "{{.TMP_ERR_LOG}}" \ - | md5sum > "{{.CHECKSUM_FILE}}" \ + 2> "{{.TMP_ERR_LOG}}" \ ; then - rm "{{.CHECKSUM_FILE}}" + rm -f "{{.CHECKSUM_FILE}}" {{- if eq "true" .FAIL}} - printf "[{{.TASK}} error] failed with:\n%s\n" "$(cat {{.TMP_ERR_LOG}})" + printf "[{{.TASK}} error] tar failed with:\n%s\n" "$(cat "{{.TMP_ERR_LOG}}")" exit 1 {{- else}} exit 0 {{- end}} fi + if ! {{.MD5_CMD}} "{{.TMP_TAR_FILE}}" > "{{.TMP_CHECKSUM_FILE}}"; then + printf "[{{.TASK}} error] failed to compute checksum for %s\n" "{{.TMP_TAR_FILE}}" + exit 1 + fi + mv -f "{{.TMP_CHECKSUM_FILE}}" "{{.CHECKSUM_FILE}}"Add the supporting vars:
vars: @@ TMP_ERR_LOG: "{{.CHECKSUM_FILE}}.log.tmp" + TMP_TAR_FILE: "{{.CHECKSUM_FILE}}.tar.tmp" + TMP_CHECKSUM_FILE: "{{.CHECKSUM_FILE}}.tmp"
23-24: Make the checksum tool portable via MD5_CMD and use it in commandsOn macOS,
md5sumoften exists only via coreutils gnubin or asgmd5sum; falling back tomd5 -rimproves portability.vars: @@ - TMP_ERR_LOG: "{{.CHECKSUM_FILE}}.log.tmp" + TMP_ERR_LOG: "{{.CHECKSUM_FILE}}.log.tmp" + MD5_CMD: + sh: | + if command -v md5sum >/dev/null 2>&1; then echo md5sum; + elif command -v gmd5sum >/dev/null 2>&1; then echo gmd5sum; + elif command -v md5 >/dev/null 2>&1; then echo 'md5 -r'; + else echo 'false'; fi @@ - | md5sum > "{{.CHECKSUM_FILE}}" \ + | {{.MD5_CMD}} > "{{.CHECKSUM_FILE}}" \Also applies to: 58-59
84-87: Validation: accumulate errors and append logs; ensure any failure flips the subshell statusAt present, each missing include overwrites the log and the loop doesn’t guarantee a final non-zero when earlier iterations failed; also use append and printf for multi-line safety.
- - defer: |- - rm -f "{{.TMP_CHECKSUM_FILE}}" - rm -f "{{.TMP_ERR_LOG}}" + - defer: |- + rm -f "{{.TMP_CHECKSUM_FILE}}" "{{.TMP_ERR_LOG}}" - |- ( - {{- range .INCLUDE_PATTERNS}} - for path in {{.}}; do - test -e "${path}" \ - || ( - echo "Include path does not exist: ${path}" > "{{$.TMP_ERR_LOG}}" - exit 1 - ) - done && \ - {{- end}} - ( - cmp -s "{{.TMP_CHECKSUM_FILE}}" "{{.CHECKSUM_FILE}}" \ - || ( - echo "cmp failed for '{{.TMP_CHECKSUM_FILE}}' '{{.CHECKSUM_FILE}}'" \ - > "{{.TMP_ERR_LOG}}" - exit 1 - ) - ) + fail=0 + : > "{{.TMP_ERR_LOG}}" + {{- range .INCLUDE_PATTERNS}} + for path in {{.}}; do + if [ ! -e "$path" ]; then + printf 'Include path does not exist: %s\n' "$path" >> "{{$.TMP_ERR_LOG}}" + fail=1 + fi + done + {{- end}} + if ! cmp -s "{{.TMP_CHECKSUM_FILE}}" "{{.CHECKSUM_FILE}}"; then + printf "cmp failed for '%s' '%s'\n" "{{.TMP_CHECKSUM_FILE}}" "{{.CHECKSUM_FILE}}" >> "{{.TMP_ERR_LOG}}" + fail=1 + fi + test $fail -eq 0 ) \ || ( {{- if eq "true" .FAIL}} - printf "[{{.TASK}} error] failed with:\n%s\n" "$(cat {{.TMP_ERR_LOG}})" + printf "[{{.TASK}} error] failed with:\n%s\n" "$(cat "{{.TMP_ERR_LOG}}")" exit 1 {{- else}} rm -f "{{.CHECKSUM_FILE}}" {{- end}} )Also applies to: 100-130
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
README.md(2 hunks)exports/taskfiles/utils/checksum.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-26T16:37:16.718Z
Learnt from: davidlion
PR: y-scope/yscope-dev-utils#81
File: exports/taskfiles/utils/checksum.yaml:16-20
Timestamp: 2025-08-26T16:37:16.718Z
Learning: In Task (Taskfile), `OS` is a built-in function, not a variable. The correct syntax is `eq OS "darwin"` without a dot prefix. Using `.OS` would be incorrect syntax.
Applied to files:
exports/taskfiles/utils/checksum.yaml
📚 Learning: 2025-08-26T16:37:16.718Z
Learnt from: davidlion
PR: y-scope/yscope-dev-utils#81
File: exports/taskfiles/utils/checksum.yaml:16-20
Timestamp: 2025-08-26T16:37:16.718Z
Learning: Task's templating system has built-in functions like `OS` that don't require the dot prefix, unlike user-defined variables which do require the dot prefix (e.g., `.VARIABLE_NAME`).
Applied to files:
exports/taskfiles/utils/checksum.yaml
🔇 Additional comments (2)
README.md (1)
53-54: Link references LGTMThe reference-style links for coreutils and gnu-tar are correct and helpful.
exports/taskfiles/utils/checksum.yaml (1)
23-24: Correct OS check usage—good catchUsing the Task built-in function
OSwithout a dot is correct here and selects GNU tar on macOS as intended.
| TAR: "{{if eq OS \"darwin\"}}gtar{{else}}tar{{end}}" | ||
| TMP_ERR_LOG: "{{.CHECKSUM_FILE}}.log.tmp" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer capability detection over OS check for TAR
Some Linux/Alpine images ship BusyBox tar which lacks these flags. Consider preferring gtar if present, falling back to tar.
- TAR: "{{if eq OS \"darwin\"}}gtar{{else}}tar{{end}}"
+ TAR:
+ sh: |
+ if command -v gtar >/dev/null 2>&1; then echo gtar; else echo tar; fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TAR: "{{if eq OS \"darwin\"}}gtar{{else}}tar{{end}}" | |
| TMP_ERR_LOG: "{{.CHECKSUM_FILE}}.log.tmp" | |
| TAR: | |
| sh: | | |
| if command -v gtar >/dev/null 2>&1; then echo gtar; else echo tar; fi | |
| TMP_ERR_LOG: "{{.CHECKSUM_FILE}}.log.tmp" |
🤖 Prompt for AI Agents
In exports/taskfiles/utils/checksum.yaml around lines 23-24, replace the
OS-based TAR selection with a runtime capability check that prefers gtar when
present and falls back to tar; specifically, change the template expression to
run a shell command like `command -v gtar` (or `which gtar`) at runtime and use
gtar if it exists, otherwise use tar, keeping TMP_ERR_LOG unchanged.
There was a problem hiding this comment.
we will improve this once we add Linux/Alphine CIs
There was a problem hiding this comment.
@Bill-hbrhbr, understood! That makes sense to defer this until you have Linux/Alpine CI environments to properly test the capability detection. Would you like me to create a GitHub issue to track this improvement for when the CI infrastructure is ready?
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
exports/taskfiles/utils/checksum.yaml (3)
23-24: OS function usage is correct; prefer capability detection for broader portabilityConfirmed per our prior learning: OS is a built-in function (no dot), so your current expression is valid on Task. To harden across BusyBox/non-GNU environments that nonetheless ship gtar, detect gtar at runtime and fall back to tar.
Apply:
- TAR: "{{if eq OS \"darwin\"}}gtar{{else}}tar{{end}}" + TAR: + sh: | + if command -v gtar >/dev/null 2>&1; then + echo gtar + else + echo tar + fi
104-122: Validation logic: existence checks don’t affect overall status; error log gets overwritten
- test -e … || ( …; exit 1 ) happens in a subshell; the for-loop continues and the outer group’s status often reflects only the last iteration.
- Using > overwrites the log; earlier errors are lost.
Accumulate failures, append errors, and assert at the end. Also append cmp failures.
Apply:
- ( - {{- range .INCLUDE_PATTERNS}} - for path in {{.}}; do - test -e "${path}" \ - || ( - echo "Include path does not exist: ${path}" > "{{$.TMP_ERR_LOG}}" - exit 1 - ) - done && \ - {{- end}} - ( - cmp -s "{{.TMP_CHECKSUM_FILE}}" "{{.CHECKSUM_FILE}}" \ - || ( - echo "cmp failed for '{{.TMP_CHECKSUM_FILE}}' '{{.CHECKSUM_FILE}}'" \ - > "{{.TMP_ERR_LOG}}" - exit 1 - ) - ) - ) \ + ( + fail=0 + : > "{{.TMP_ERR_LOG}}" + {{- range .INCLUDE_PATTERNS}} + for path in {{.}}; do + if [ ! -e "${path}" ]; then + echo "Include path does not exist: ${path}" >> "{{$.TMP_ERR_LOG}}" + fail=1 + fi + done + {{- end}} + if ! cmp -s "{{.TMP_CHECKSUM_FILE}}" "{{.CHECKSUM_FILE}}"; then + echo "cmp failed for '{{.TMP_CHECKSUM_FILE}}' '{{.CHECKSUM_FILE}}'" >> "{{.TMP_ERR_LOG}}" + fail=1 + fi + test $fail -eq 0 + ) \
51-56: Nil-safety for EXCLUDE_PATTERNS during rangeEven if you fix vars, being defensive at the use site avoids surprises when this task is embedded under different callers.
Apply:
- {{- range .EXCLUDE_PATTERNS}} + {{- range (default (list) .EXCLUDE_PATTERNS) }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
exports/taskfiles/utils/checksum.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-26T16:37:16.718Z
Learnt from: davidlion
PR: y-scope/yscope-dev-utils#81
File: exports/taskfiles/utils/checksum.yaml:16-20
Timestamp: 2025-08-26T16:37:16.718Z
Learning: In Task (Taskfile), `OS` is a built-in function, not a variable. The correct syntax is `eq OS "darwin"` without a dot prefix. Using `.OS` would be incorrect syntax.
Applied to files:
exports/taskfiles/utils/checksum.yaml
📚 Learning: 2025-08-26T16:37:16.718Z
Learnt from: davidlion
PR: y-scope/yscope-dev-utils#81
File: exports/taskfiles/utils/checksum.yaml:16-20
Timestamp: 2025-08-26T16:37:16.718Z
Learning: Task's templating system has built-in functions like `OS` that don't require the dot prefix, unlike user-defined variables which do require the dot prefix (e.g., `.VARIABLE_NAME`).
Applied to files:
exports/taskfiles/utils/checksum.yaml
📚 Learning: 2024-10-28T19:02:02.131Z
Learnt from: AVMatthews
PR: y-scope/yscope-dev-utils#13
File: taskfiles/utils.yml:149-149
Timestamp: 2024-10-28T19:02:02.131Z
Learning: In `taskfiles/utils.yml`, avoid using GNU-specific options like `-r` and `--no-run-if-empty` with `xargs` for better portability; instead, use `find`'s `-exec` option.
Applied to files:
exports/taskfiles/utils/checksum.yaml
🔇 Additional comments (1)
exports/taskfiles/utils/checksum.yaml (1)
19-21: Critical: invalid use of ref with template functions; EXCLUDE_PATTERNS won’t resolveTask’s ref cannot evaluate template functions. As written, ref: "default (list) .EXCLUDE_PATTERNS" will not work and may break the task. Use a templated string instead so default/list are evaluated.
Apply:
- EXCLUDE_PATTERNS: - ref: "default (list) .EXCLUDE_PATTERNS" + EXCLUDE_PATTERNS: "{{ default (list) .EXCLUDE_PATTERNS }}"Likely an incorrect or invalid review comment.
| TAR: "{{if eq OS \"darwin\"}}gtar{{else}}tar{{end}}" | ||
| TMP_ERR_LOG: "{{.CHECKSUM_FILE}}.log.tmp" | ||
| requires: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add MD5_CMD and temp artifact vars for portability and atomic writes
md5sum is not present by default on macOS; Homebrew installs gmd5sum. Also, writing directly to CHECKSUM_FILE risks partial files on failure. Introduce MD5_CMD and temp files now (used in compute refactor below).
Apply:
TMP_ERR_LOG: "{{.CHECKSUM_FILE}}.log.tmp"
+ TMP_TAR_FILE: "{{.CHECKSUM_FILE}}.tar.tmp"
+ TMP_CHECKSUM_FILE: "{{.CHECKSUM_FILE}}.tmp"
+ MD5_CMD:
+ sh: |
+ if command -v gmd5sum >/dev/null 2>&1; then
+ echo gmd5sum
+ elif command -v md5sum >/dev/null 2>&1; then
+ echo md5sum
+ elif command -v md5 >/dev/null 2>&1; then
+ # macOS fallback; -r prints digest first to match *md5sum output style
+ echo 'md5 -r'
+ else
+ echo 'false' # no-op that will fail
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TAR: "{{if eq OS \"darwin\"}}gtar{{else}}tar{{end}}" | |
| TMP_ERR_LOG: "{{.CHECKSUM_FILE}}.log.tmp" | |
| requires: | |
| TAR: "{{if eq OS \"darwin\"}}gtar{{else}}tar{{end}}" | |
| TMP_ERR_LOG: "{{.CHECKSUM_FILE}}.log.tmp" | |
| TMP_TAR_FILE: "{{.CHECKSUM_FILE}}.tar.tmp" | |
| TMP_CHECKSUM_FILE: "{{.CHECKSUM_FILE}}.tmp" | |
| MD5_CMD: | |
| sh: | | |
| if command -v gmd5sum >/dev/null 2>&1; then | |
| echo gmd5sum | |
| elif command -v md5sum >/dev/null 2>&1; then | |
| echo md5sum | |
| elif command -v md5 >/dev/null 2>&1; then | |
| # macOS fallback; -r prints digest first to match *md5sum output style | |
| echo 'md5 -r' | |
| else | |
| echo 'false' # no-op that will fail | |
| fi | |
| requires: |
🤖 Prompt for AI Agents
In exports/taskfiles/utils/checksum.yaml around lines 23-25, add a
platform-aware MD5_CMD variable and explicit temp artifact vars for atomic
writes: define MD5_CMD to use gmd5sum on darwin and md5sum otherwise, and add
CHECKSUM_TMP (e.g. "{{.CHECKSUM_FILE}}.tmp") and retain TMP_ERR_LOG (or rename
to CHECKSUM_ERR_LOG if desired) so compute steps can write to temp files and
then atomically mv to CHECKSUM_FILE; update downstream steps to use these new
vars for generating checksums and error logs.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
taskfiles/tests.yaml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests (macos-15)
- GitHub Check: unit-tests (ubuntu-24.04)
- GitHub Check: unit-tests (ubuntu-22.04)
🔇 Additional comments (1)
taskfiles/tests.yaml (1)
5-5: Include verified — default task presentConfirmed taskfiles/checksum/tests.yaml defines a
defaulttask (line 9).
| TAR: "{{if eq OS \"darwin\"}}gtar{{else}}tar{{end}}" | ||
| TMP_ERR_LOG: "{{.CHECKSUM_FILE}}.log.tmp" |
There was a problem hiding this comment.
we will improve this once we add Linux/Alphine CIs
Co-authored-by: Bingran Hu <[email protected]>
| # @param {string[]} INCLUDE_PATTERNS Path wildcard patterns to compute the checksum for. | ||
| # @param {string[]} [EXCLUDE_PATTERNS] Path wildcard patterns, relative to any `INCLUDE_PATTERNS`, | ||
| # to exclude from the checksum. | ||
| # @param {string} [FAIL="true"] If set to "false" the task will not fail. |
There was a problem hiding this comment.
| # @param {string} [FAIL="true"] If set to "false" the task will not fail. | |
| # @param {string} [FAIL="true"] If set to "false", the task will not fail. |
| shopt: ["globstar"] | ||
|
|
||
| tasks: | ||
|
|
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
generally lgtm. posted some suggestions like renaming the var FAIL and some other nitpicking comments
| ; then | ||
| rm -f "{{.CHECKSUM_FILE}}" | ||
| {{- if eq "true" .FAIL}} | ||
| printf "[{{.TASK}} error] failed with:\n%s\n" "$(cat {{.TMP_ERR_LOG}})" |
There was a problem hiding this comment.
| printf "[{{.TASK}} error] failed with:\n%s\n" "$(cat {{.TMP_ERR_LOG}})" | |
| printf "[{{.TASK}} error] failed with:\n%s\n" "$(cat '{{.TMP_ERR_LOG}}')" |
| # @param {string[]} INCLUDE_PATTERNS Path wildcard patterns to compute the checksum for. | ||
| # @param {string[]} [EXCLUDE_PATTERNS] Path wildcard patterns, relative to any `INCLUDE_PATTERNS`, | ||
| # to exclude from the checksum. | ||
| # @param {string} [FAIL="true"] If set to "false" the task will not fail. |
There was a problem hiding this comment.
Would it be better to change this into IGNORE_ERROR with default = false?
- the name
FAILcan be confusing. i feelIGNORE_ERRORcan be more self-explanatory - it's usually better to use
falseas the default value of any Booleans - we can early return right after
rm -f "{{.CHECKSUM_FILE}}"
| # @param {string[]} INCLUDE_PATTERNS Path wildcard patterns to validate the checksum for. | ||
| # @param {string[]} [EXCLUDE_PATTERNS] Path wildcard patterns, relative to any `INCLUDE_PATTERNS`, | ||
| # to exclude from the checksum. | ||
| # @param {string} [FAIL="false"] If set to "true", the task fails when checksums mismatch. |
There was a problem hiding this comment.
would FAIL_ON_ERROR be more self-explanatory?
| # different from the original. | ||
| - "test -e '{{.FILE_0}}'" | ||
| - "test ! -e '{{.FILE_1}}'" | ||
| - "cmp -s '{{.FILE_0}}' '{{.CHECKSUM_FILE_REF0}}'" |
There was a problem hiding this comment.
| - "cmp -s '{{.FILE_0}}' '{{.CHECKSUM_FILE_REF0}}'" | |
| - "cmp -s '{{.FILE_0}}' '{{.CHECKSUM_FILE_REF0}}'" |
| {{.ARCHIVER}} \ | ||
| --create \ | ||
| --file - \ | ||
| --group 0 \ | ||
| --mtime "UTC 1970-01-01" \ | ||
| --numeric-owner \ | ||
| --owner 0 \ | ||
| --sort name \ | ||
| --no-anchored \ | ||
| --wildcards \ | ||
| {{- range .EXCLUDE_PATTERNS}} | ||
| --exclude="{{.}}" \ | ||
| {{- end}} | ||
| {{- range .INCLUDE_PATTERNS}} | ||
| {{.}} \ | ||
| {{- end}} | ||
| 2> "{{.TMP_ERR_LOG}}" \ | ||
| | md5sum > "{{.CHECKSUM_FILE}}" \ |
There was a problem hiding this comment.
indent
| {{.ARCHIVER}} \ | |
| --create \ | |
| --file - \ | |
| --group 0 \ | |
| --mtime "UTC 1970-01-01" \ | |
| --numeric-owner \ | |
| --owner 0 \ | |
| --sort name \ | |
| --no-anchored \ | |
| --wildcards \ | |
| {{- range .EXCLUDE_PATTERNS}} | |
| --exclude="{{.}}" \ | |
| {{- end}} | |
| {{- range .INCLUDE_PATTERNS}} | |
| {{.}} \ | |
| {{- end}} | |
| 2> "{{.TMP_ERR_LOG}}" \ | |
| | md5sum > "{{.CHECKSUM_FILE}}" \ | |
| {{.ARCHIVER}} \ | |
| --create \ | |
| --file - \ | |
| --group 0 \ | |
| --mtime "UTC 1970-01-01" \ | |
| --numeric-owner \ | |
| --owner 0 \ | |
| --sort name \ | |
| --no-anchored \ | |
| --wildcards \ | |
| {{- range .EXCLUDE_PATTERNS}} | |
| --exclude="{{.}}" \ | |
| {{- end}} | |
| {{- range .INCLUDE_PATTERNS}} | |
| {{.}} \ | |
| {{- end}} | |
| 2> "{{.TMP_ERR_LOG}}" \ | |
| | md5sum > "{{.CHECKSUM_FILE}}" \ |
There was a problem hiding this comment.
or maybe the 4-space indents are intentional? i can't be sure what are the conventions when we mix shell commands with go templates...
the code is fairly readable either way anyways
| || ( | ||
| echo "Include path does not exist: ${path}" > "{{$.TMP_ERR_LOG}}" | ||
| exit 1 | ||
| ) |
There was a problem hiding this comment.
| || ( | |
| echo "Include path does not exist: ${path}" > "{{$.TMP_ERR_LOG}}" | |
| exit 1 | |
| ) | |
| || ( | |
| echo "Include path does not exist: ${path}" > "{{$.TMP_ERR_LOG}}" | |
| exit 1 | |
| ) |
| ( | ||
| cmp -s "{{.TMP_CHECKSUM_FILE}}" "{{.CHECKSUM_FILE}}" \ | ||
| || ( | ||
| echo "cmp failed for '{{.TMP_CHECKSUM_FILE}}' '{{.CHECKSUM_FILE}}'" \ | ||
| > "{{.TMP_ERR_LOG}}" | ||
| exit 1 | ||
| ) | ||
| ) |
There was a problem hiding this comment.
| ( | |
| cmp -s "{{.TMP_CHECKSUM_FILE}}" "{{.CHECKSUM_FILE}}" \ | |
| || ( | |
| echo "cmp failed for '{{.TMP_CHECKSUM_FILE}}' '{{.CHECKSUM_FILE}}'" \ | |
| > "{{.TMP_ERR_LOG}}" | |
| exit 1 | |
| ) | |
| ) | |
| ( | |
| cmp -s "{{.TMP_CHECKSUM_FILE}}" "{{.CHECKSUM_FILE}}" \ | |
| || ( | |
| echo "cmp failed for '{{.TMP_CHECKSUM_FILE}}' '{{.CHECKSUM_FILE}}'" \ | |
| > "{{.TMP_ERR_LOG}}" | |
| exit 1 | |
| ) | |
| ) |
| || ( | ||
| {{- if eq "true" .FAIL}} | ||
| printf "[{{.TASK}} error] failed with:\n%s\n" "$(cat {{.TMP_ERR_LOG}})" | ||
| exit 1 | ||
| {{- else}} | ||
| rm -f "{{.CHECKSUM_FILE}}" | ||
| {{- end}} | ||
| ) |
There was a problem hiding this comment.
| || ( | |
| {{- if eq "true" .FAIL}} | |
| printf "[{{.TASK}} error] failed with:\n%s\n" "$(cat {{.TMP_ERR_LOG}})" | |
| exit 1 | |
| {{- else}} | |
| rm -f "{{.CHECKSUM_FILE}}" | |
| {{- end}} | |
| ) | |
| || ( | |
| {{- if eq "true" .FAIL}} | |
| printf "[{{.TASK}} error] failed with:\n%s\n" "$(cat {{.TMP_ERR_LOG}})" | |
| exit 1 | |
| {{- else}} | |
| rm -f "{{.CHECKSUM_FILE}}" | |
| {{- end}} | |
| ) |
3 participants
Description
This PR addresses a few issues with the
checksumtasks and also adds unit tests for these tasks.checksum:computerequires gnu tar for its flags and would fail silently on MacOS (unlesstarwas set to gnu tar)gtarif running on macos.FAILflag allowing users to change whether they wish to allow the task to fail or not.For
compute,FAILdefaults totrueas there is currently only one case where we do not want to fail. When we callcomputefromvalidatewe want task to continue ifcomputefails asvalidatewill delete the checksum file so that the calling task will not be skipped (assuming it correctly includes the checksum in itsgeneratesfield). Previously,computeusedignore_error:trueto blindly succeed in all cases.For
validate,FAILdefaults tofalseas generally the "failing" workflow is forvalidateto delete the checksum so that the caller runs and (re-)creates all the content for the checksum. In the new workflow, where we "snapshot" different stages in a build by computing checksums at each stage, a caller may need to validate a checksum which it will not (re-)create the content of (e.g. abuildtask will want to validate the checksum fromgenerate, butbuildis not capable of (re-)runninggenerate). In this case,validateshould cause task to fail as the caller has no way to recover.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Documentation
Tests
Chores