Count field defaults to 1 if not provided #4869
Conversation
WalkthroughThe pull request introduces a custom JSON unmarshaling method for the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant J as Job Model
C->>J: Submit JSON payload with "Count"
J->>J: Execute custom UnmarshalJSON
alt Batch/Service Job
J->>J: If Count omitted, set to 1
J->>J: If Count explicitly 0, mark to stop executions
else Daemon/Ops Job
J->>J: Force Count to 0, validate errors for Count > 1
end
J->>C: Return processed Job instance
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2){"Issues":[{"FromLinter":"typecheck","Text":"pattern build/: no matching files found","Severity":"","SourceLines":["//go:embed build/"],"Replacement":null,"Pos":{"Filename":"webui/webui.go","Offset":0,"Line":23,"Column":12},"ExpectNoLint":false,"ExpectedNoLintLinter":""}],"Report":{"Linters":[{"Name":"asasalint"},{"Name":"asciicheck"},{"Name":"bidichk"},{"Name":"bodyclose","Enabled":true},{"Name":"canonicalheader"},{"Name":"containedctx"},{"Name":"contextcheck"},{"Name":"copyloopvar","Enabled":true},{"Name":"cyclop"},{"Name":"decorder"},{"Name":"deadcode"},{"Name":"depguard","Enabled":true},{"Name":"dogsled","Enabled":true},{"Name":"dupl"},{"Name":"dupword"},{"Name":"durationcheck"},{"Name":"errcheck","Enabled":true,"EnabledByDefault":true},{"Name":"errchkjson"},{"Name":"errname"},{"Name":"errorlint"},{"Name":"execinquery"},{"Name":"exhaustive"},{"Name":"exhaustivestruct"},{"Name":"exhaustruct"},{"Name":"exportloopref"},{"Name":"forbidigo","Enabled":true},{"Name":"forcetypeassert"},{"Name":"fatcontext"},{"Name":"funlen","Enabled":true},{"Name":"gci"},{"Name":"ginkgolinter"},{"Name":"gocheckcompilerdirectives"},{"Name":"gochecknoglobals"},{"Name":"gochecknoinits","Enabled":true},{"Name":"gochecksumtype"},{"Name":"gocognit"},{"Name":"goconst","Enabled":true},{"Name":"gocritic"},{"Name":"gocyclo","Enabled":true},{"Name":"godot"},{"Name":"godox"},{"Name":"err113"},{"Name":"gofmt","Enabled":true},{"Name":"gofumpt"},{"Name":"goheader"},{"Name":"goimports","Enabled":true},{"Name":"golint"},{"Name":"mnd","Enabled":true},{"Name":"gomnd"},{"Name":"gomoddirectives"},{"Name":"gomodguard"},{"Name":"goprintffuncname","Enabled":true},{"Name":"gosec","Enabled":true},{"Name":"gosimple","Enabled":true,"EnabledByDefault":true},{"Name":"gosmopolitan"},{"Name":"govet","Enabled":true,"EnabledByDefault":true},{"Name":"grouper"},{"Name":"ifshort"},{"Name":"iface"},{"Name":"importas"},{"Name":"inamedparam"},{"Name":"ineffassign","Enabled":true,"EnabledByDefault":true},{"Name":"interfacebloat"},{"Name":"interfacer"},{"Name":"intrange"},{"Name":"ireturn"},{"Name":"lll","Enabled":true},{"Name":"loggercheck"},{"Name":"maintidx"},{"Name":"makezero"},{"Name":"maligned"},{"Name":"mirror"},{"Name":"misspell"},{"Name":"musttag"},{"Name":"nakedret","Enabled":true},{"Name":"nestif"},{"Name":"nilerr"},{"Name":"nilnil"},{"Name":"nlreturn"},{"Name":"noctx","Enabled":true},{"Name":"nonamedreturns"},{"Name":"nosnakecase"},{"Name":"nosprintfhostport"},{"Name":"paralleltest"},{"Name":"perfsprint"},{"Name":"prealloc"},{"Name":"predeclared"},{"Name":"promlinter"},{"Name":"protogetter"},{"Name":"reassign"},{"Name":"recvcheck"},{"Name":"revive"},{"Name":"rowserrcheck"},{"Name":"sloglint"},{"Name":"scopelint"},{"Name":"sqlclosecheck"},{"Name":"spancheck"},{"Name":"staticcheck","Enabled":true,"EnabledByDefault":true},{"Name":"structcheck"},{"Name":"stylecheck","Enabled":true},{"Name":"tagalign"},{"Name":"tagliatelle"},{"Name":"tenv"},{"Name":"testableexamples"},{"Name":"testifylint"},{"Name":"testpackage"},{"Name":"thelper"},{"Name":"tparallel"},{"Name":"typecheck","Enabled":true,"EnabledByDefault":true},{"Name":"unconvert","Enabled":true},{"Name":"unparam"},{"Name":"unused","Enabled":true,"EnabledByDefault":true},{"Name":"usestdlibvars"},{"Name":"varcheck"},{"Name":"varnamelen"},{"Name":"wastedassign"},{"Name":"whitespace","Enabled":true},{"Name":"wrapcheck"},{"Name":"wsl"},{"Name":"zerologlint"},{"Name":"nolintlint","Enabled":true}]}} ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/swagger/swagger.json (1)
1699-1702: Documentation Clarity and Consistency for "Count" FieldThe updated description for the
"Count"property in the"models.Job"definition is clear and comprehensive. It explains that for batch and service jobs:
- If the count is not provided in the JSON, it defaults to 1.
- An explicit value of 0 stops all executions.
- A value greater than 0 specifies the exact number of replicas.
It also correctly details that for daemon and ops jobs:
- Values of 0 or 1 are ignored (i.e., the job runs on all matching nodes),
- Values greater than 1 are invalid and will fail validation.
This update aligns well with the PR objectives to improve job submission usability and prevent silent failures when the count is omitted.
Suggestion: If the Swagger UI supports enhanced formatting, consider using markdown bullet points or line breaks in the description for improved readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/models/job.go(5 hunks)pkg/models/job_test.go(3 hunks)pkg/swagger/docs.go(1 hunks)pkg/swagger/swagger.json(1 hunks)webui/lib/api/schema/swagger.json(1 hunks)
🔇 Additional comments (11)
pkg/swagger/docs.go (1)
1704-1704: Clarified job Count specifications in Swagger docs.
This updated description matches the newly introduced logic for handling theCountfield across different job types.pkg/models/job.go (5)
5-5: Approved import for JSON handling.
Importing"encoding/json"is essential for the customUnmarshalJSONmethod introduced below.
103-110: Accurate and thorough Count field documentation.
These comments align well with the intended behavior for batch/service vs. daemon/ops job types.
192-195: Correct default enforcement for daemon and ops jobs.
SettingCount = 0for these job types prevents unintended replicas and aligns with the documentation.
270-272: Validation prevents invalid replica counts for daemon/ops.
Raising an error forCount > 1on daemon/ops jobs is consistent with the documented restrictions.
411-443: Well-structured custom JSON unmarshal for Count defaults.
This logic correctly defaultsCountto 1 for batch/service when omitted, and preserves explicit zero while deferring other cases to validation.webui/lib/api/schema/swagger.json (1)
700-701: Clear and comprehensive documentation update for the Count field.The updated description clearly explains the behavior of the Count field for different job types, which aligns with the PR objectives of improving user experience by defaulting batch and service jobs to a count of 1 when not specified.
pkg/models/job_test.go (4)
20-106: Well-structured test cases for job normalization.The test cases are well organized with descriptive names and cover all important scenarios for different job types:
- Batch and service jobs retaining their original count values (0 or positive)
- Ops and daemon jobs being forced to count=0 regardless of input value
Good job on refactoring the tests to be more readable and comprehensive.
108-186: Comprehensive validation test cases.The test cases now properly validate important constraints:
- Negative count values are rejected
- Daemon jobs cannot have count > 1
- Ops jobs cannot have count > 1
This ensures the system enforces the rules documented in the Swagger file.
280-380: Excellent JSON handling test coverage.This new test function is a crucial addition that verifies the JSON unmarshaling behavior which implements the core feature of this PR - defaulting to count=1 for batch and service jobs when count is omitted. It thoroughly tests:
- All job types (batch, service, daemon, ops)
- Explicit count values (0, positive numbers)
- Omitted count field behavior
- Default value application during unmarshaling
The test structure makes it easy to understand the expected behavior for each scenario.
382-385: Clean utility function implementation.This helper function simplifies the test code by providing a concise way to create integer pointers, which are needed for the JSON marshaling tests where some fields might be omitted.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/models/job.go (1)
412-444: Good custom unmarshaling approach; consider a single-source defaulting.The logic here correctly applies defaults for batch/service jobs and preserves explicit zero values. Since you also override
Countfor daemon/ops jobs inNormalize(), you may consider consolidating all defaults or overrides in one location to reduce complexity, although this is optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/lib/marshaller/utils_test.go(1 hunks)pkg/models/job.go(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/lib/marshaller/utils_test.go
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build / Build Binary (windows, amd64)
- GitHub Check: build / Build Binary (darwin, arm64)
- GitHub Check: build / Build Binary (darwin, amd64)
- GitHub Check: build / Build Binary (linux, armv6)
- GitHub Check: build / Build Binary (linux, armv7)
- GitHub Check: build / Build Binary (linux, arm64)
- GitHub Check: build / Build Binary (linux, amd64)
🔇 Additional comments (4)
pkg/models/job.go (4)
5-5: No issues with new import.The addition of
encoding/jsonfor custom unmarshaling is appropriate and consistent with the implementation that follows.
104-110: Documentation is clear and consistent.These doc comments accurately explain the intended behavior for the
Countfield across different job types.
192-196: Consistent override for daemon and ops jobs.Forcing
Countto 0 at normalization ensures that daemon and ops jobs behave consistently, regardless of any user-supplied value.
270-272: Validation aligns with specification.Restricting
Countto 1 or less for daemon and ops jobs prevents invalid configurations, matching the documented behavior.
Currently when users submit a job without specifying a count, it's not clear what happens:
This change makes it more intuitive:
This should reduce confusion and make job submission more user-friendly, especially
for new users trying out the system.
Examples
Job will run and default to count=1
Summary by CodeRabbit
New Features
Documentation
Tests