feat(config): validate jobs, not just the global section - #778
Conversation
Closes #773. The validator walked structs and skipped maps. Every job lives in one, so no job was ever reachable by it: an unparsable schedule passed validate with the strict flag on or off, and the only later sign was a warning while the daemon started and the job never fired. Two things were needed to reach a job's fields. Job sections are maps, so the walk descends into them; and every job type embeds core.<Kind>Job which embeds BareJob, where schedule and command live, so squashed structs are descended into as well. The latter only inside a job — the global section keeps the behavior it had, and the shipped example, the run-job example and the test config all still validate clean. Applying the existing rule to jobs would have been the wrong kind of strict. "A field with no default tag is required" is tuned to the global section; on a job it would demand nearly every key a job can carry and reject configurations that work today. That rule is therefore suppressed inside jobs, and what a job needs is stated explicitly, taken from what the runtime already demands rather than invented here: job-exec container, command job-run image or container (core.RunJob.Validate) job-service-run image (core.RunServiceJob.Validate) job-local command job-compose file, service all schedule Errors name the section and the job the user wrote, so "job-exec \"foo\": container is required (the container to exec in)" points at the line to fix rather than at a bare key. One existing fixture had to change rather than the rule: TestValidate- ExecuteValidFile declared a job-exec with no container and asserted the config was valid. It is not — verified against the daemon, that job fails on every single tick with `run_exec container "": invalid container name or ID: value is empty`. The test was pinning a config that cannot work. The keys named in both the format switch and the requirements table are now constants; before the table they existed once, and duplicating them with nothing tying the two lists together is what goconst was pointing at. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Mutation Testing ResultsMutation Score: 100.00% (threshold: 60%)
What is mutation testing?Mutation testing measures test quality by introducing small changes (mutations) to the code and checking if tests detect them. A higher score means better test effectiveness.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #778 +/- ##
==========================================
- Coverage 89.31% 89.29% -0.02%
==========================================
Files 88 88
Lines 12071 12128 +57
==========================================
+ Hits 10781 10830 +49
- Misses 995 999 +4
- Partials 295 299 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a long-standing gap in ofelia validate by ensuring the config validator can actually reach and validate per-job fields (not just the global section). It extends the validator’s reflection walk to traverse job maps and (only within jobs) descend into mapstructure:",squash" embedded structs so that shared fields like schedule/command are validated, and it adds explicit per-job-kind required-field checks aligned with runtime requirements.
Changes:
- Traverse job section maps during validation so job entries are reachable by the validator.
- Within job entries only, descend into squashed embedded structs and suppress the global “no default tag ⇒ required” heuristic; instead enforce explicit per-kind job requirements.
- Add unit + e2e coverage to pin schedule parsing failures and missing required job fields, while ensuring the shipped example config still validates.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
config/validator.go |
Extends the validator walk to descend into job maps and squashed embedded structs within jobs; adds explicit job requirements and qualified job error paths. |
config/validator_jobs_test.go |
New unit tests covering job traversal, schedule parsing, per-kind requirements, and the “don’t require every field” rule inside jobs. |
config/validator_mutation_test.go |
Updates tests for the new validateStringField signature using fieldCtx. |
config/validator_boundary_test.go |
Updates boundary tests for the new validateStringField signature using fieldCtx. |
e2e/config_validation_test.go |
Adds end-to-end validation tests for unparsable schedules, missing job requirements, and continued acceptance of example/ofelia.ini. |
cli/validate_test.go |
Fixes the test fixture to use a runnable job-exec by adding the missing container field. |
Closes #773. Depends on nothing; complements #777, which fixed the daemon half.
The validator never saw a job
It walks structs and skipped maps — and every job lives in one. So an unparsable schedule passed
validatewith the strict flag on or off, and the only later sign was a warning while the daemon started and the job never fired.Two things were needed to reach a job's fields:
core.<Kind>Job, which embedsBareJob— wherescheduleandcommandlive. Squashed structs were skipped too, so even after traversing the maps the schedule stayed invisible. They are now descended into only inside a job, so the global section keeps the behaviour it had.Strict, but not the wrong kind of strict
Applying the existing rule to jobs would have rejected configurations that work today. "A field with no
defaulttag is required" is a heuristic tuned to the global section; on a job it demands nearly every key a job can carry.So that rule is suppressed inside jobs, and what a job needs is stated explicitly — taken from what the runtime already demands, not invented here:
job-execcontainer,commandRunExec(ctx, j.Container, …)job-runimageorcontainercore.RunJob.Validate→ErrImageOrContainerjob-service-runimagecore.RunServiceJob.Validate→ErrImageRequiredjob-localcommandargs.GetArgs(j.Command)job-composefile,servicedocker compose -f … runit buildsscheduleErrors name the section and the job the user wrote:
Impact measured, not assumed
Every config in the repo still validates clean:
example/ofelia.ini,test/test-config.ini,test/run-job/ofelia.ini. There is an e2e test pinning the shipped example specifically, so adding job checks cannot start rejecting the file new users are handed.One existing fixture had to change rather than the rule.
TestValidateExecuteValidFiledeclared ajob-execwith nocontainerand asserted the config was valid. It is not — verified against the running daemon, that job fails on every tick:The test was pinning a config that cannot work. This is precisely the class of failure the change exists to catch, so the fixture gained a container.
Note on the constants
command,image,containerandscheduleare now constants. They were literals in the format switch, which carried a comment arguing that extracting them only relocates duplication — true while they appeared once. The requirements table makes them appear in two independent lists with nothing tying them together, which is what goconst was pointing at.Test plan
go test ./...— greengo test -race -tags=e2e ./e2e/...— greengolangci-lint runincl.--build-tags="e2e unix"— 0 issueslefthook run pre-push— exit 0job-runwith only a container)