[CFX-7834] refactor(task): validate --concurrency at parse time - #860
Merged
ajalon1 merged 1 commit intoAug 31, 2026
Merged
Conversation
|
🎫 Jira: |
ajalon1
force-pushed
the
aj/CFX-7834-countflags-task
branch
from
August 27, 2026 20:49
fa0bc1a to
4f5ba27
Compare
ajalon1
marked this pull request as ready for review
August 27, 2026 21:09
chasdr
approved these changes
Aug 27, 2026
chasdr
left a comment
chasdr
left a comment
Contributor
There was a problem hiding this comment.
👋 chasbot here, chas had me take a pass at this one.
clean. -C 0 quietly meaning "unlimited" was a real trap, and pushing the check to parse time via countflags.PositiveInt kills it before the task runner is even looked up. default 2 is preserved, Type() still reports int so GetInt keeps working, and the reject test needs no task env because parsing dies first. tidy work, you and your droid.
built the two packages and ran the new test on the branch, both green. approving with the stack in mind (this rides on #858/#859).
LGTM 👍
Migrates dr task run --concurrency onto internal/countflags.PositiveInt (default 2 unchanged). Zero or negative values previously reached the task runner and ran nothing or misbehaved; they are now rejected during flag parsing with cobra's standard "invalid argument" message. Adds a reject test in the existing command-test style. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
ajalon1
force-pushed
the
aj/CFX-7834-countflags-task
branch
from
August 31, 2026 17:34
4f5ba27 to
09314f0
Compare
Contributor
Author
|
/approve-smoke-tests |
ajalon1
added a commit
to ajalon1/derkeley
that referenced
this pull request
Aug 31, 2026
Today's countflags migration (CFX-7834, datarobot-oss#858/datarobot-oss#859/datarobot-oss#860) is documented in docs/development/flags.md but had no reviewer-facing rule — a new count-like flag validated in RunE instead of at parse time would sail through review. Adds the parse-time-validation rule plus its documented exception (0 as a "use default" sentinel, per the workload config --port/--replicas precedent).
cdevent
pushed a commit
that referenced
this pull request
Sep 3, 2026
Migrates dr task run --concurrency onto internal/countflags.PositiveInt (default 2 unchanged). Zero or negative values previously reached the task runner and ran nothing or misbehaved; they are now rejected during flag parsing with cobra's standard "invalid argument" message. Adds a reject test in the existing command-test style. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
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
2 participants
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.
RATIONALE
dr task run --concurrencyforwarded any value verbatim as-C <n>to the task binary, and two of those values did the opposite of what a user would expect:--concurrency 0meant unlimited parallelism, not "use the default 2". go-task treats-C 0as "no concurrency limit", so passing 0 — a reasonable reading of "default" — unbounded the fan-out across every task in the graph. With recursive/root Taskfiles that is a quiet resource spike.-C -1), leaving its behavior undefined rather than ours.Both now die at parse time via
internal/countflags.PositiveInt(from #858): zero concurrency would run nothing, so it is rejected with the standardinvalid argumentmessage, before the task runner is even looked up.CHANGES
--concurrency->countflags.PositiveInt(default 2 unchanged).TESTING
task lint(all GOOS legs) andtask test(race + coverage) pass.NOTES
Part 3/4 of a stacked review. Based on #859, so this diff shows only the task change.