You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Read changed files and understand the implementation
Analyze code for bugs, edge cases, and quality issues
Post review feedback
Summary: The feature adds replace_args to YAML service configs, allowing flag-value pair replacement without redefining the entire arg list. The implementation is clean and the test coverage is decent, but there's one bug that needs fixing:
Bug — panic on odd-length replaceArgs (recipe_yaml.go:485-487): When replaceArgs has an odd number of elements, the code logs a warning but continues into the loop. The replaceArgs[i+1] access on the next iteration will panic with index out-of-range. The fix is to truncate replaceArgs to an even length after the warning, or return early. See inline comment →
Minor concerns:
No enforcement when both args and replace_args are set on the same service — the current behavior silently applies both in sequence, which may surprise users. See inline comment →
Re-review: Arg replacement for custom recipes (post-sync)
Read updated changed files and diff
Check if previous review issues were addressed
Post updated review feedback
Summary: The latest commit (30eca91) only addresses lint style — combining parameter declarations. The previously reported issues remain:
Bug — panic on odd-length replaceArgs (recipe_yaml.go:485-487): Still present. The warning logs but execution continues into the loop, where replaceArgs[i+1] will panic on the last iteration. See inline comment →
No test for odd-length edge case: Still missing. Adding one would catch the panic and serve as a regression test.
No enforcement when both args and replace_args are set: Still a minor concern (silent interaction), though not a blocker.
Re-review: Arg replacement for custom recipes (post-sync)
Read updated changed files and diff
Check if previous review issues were addressed
Post updated review feedback
What changed: Commit bd6a114 adds validation that args and replace_args are mutually exclusive in cmd_validate.go. This addresses concern #3 from previous reviews.
Remaining issues:
Bug — panic on odd-length replaceArgs (recipe_yaml.go:485-487): Still present (third review flagging this). The warning logs but execution continues into the loop, where replaceArgs[i+1] will panic. Fix is one line: truncate to even length after the warning. See inline comment →
No test for odd-length edge case: Still missing. Would serve as a regression test for the panic.
Validation is opt-in only: The new args/replace_args mutual exclusivity check in cmd_validate.go only runs when a user explicitly runs the validate subcommand. During normal cook execution, both are still silently applied in sequence (recipe_yaml.go:437-442). Consider either enforcing this at apply-time too, or documenting that validate should be run before cook. This is a minor concern, not a blocker.
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
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.
This allows replacing flags like:
with:
without redefining the entire flag set. It is not used in case the custom recipe prefers definining the full set of args with
args.(This was considered for supporting the buildernet recipe but is no longer necessary for that. It is still a potentially useful feature.)