feat(core): ExecJob console-height / console-width for TTY sizing (#235) - #701
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
✅ 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.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #701 +/- ##
==========================================
- Coverage 88.10% 88.08% -0.03%
==========================================
Files 89 89
Lines 11497 11504 +7
==========================================
+ Hits 10130 10133 +3
- Misses 1115 1117 +2
- Partials 252 254 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds operator-configurable initial pseudo-TTY console size (console-height / console-width) to job-exec, plumbed through domain.ExecConfig to Docker's ContainerExecCreate.ConsoleSize. Defaults preserve pre-fix behavior (nil → Docker default).
Changes:
- Add
ConsoleHeight/ConsoleWidthfields onExecJoband aconsoleSize()helper that returns nil when both are zero, populatingExecConfig.ConsoleSizeon bothRunandRunWithStreamspaths. - Extend
domain.ExecConfigwithConsoleSize *[2]uintand forward it through the Docker adapter tocontainertypes.ExecOptions. - Add unit/integration tests pinning [height, width] order, partial-population semantics, and the nil-default backward-compat path; CHANGELOG entry under Added.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| core/execjob.go | New ConsoleHeight/ConsoleWidth fields, consoleSize() helper, and wiring into both exec config builders. |
| core/domain/exec.go | Adds ConsoleSize *[2]uint to the domain ExecConfig. |
| core/adapters/docker/exec.go | Propagates config.ConsoleSize to containertypes.ExecOptions. |
| core/execjob_console_size_test.go | New tests covering nil default, [height,width] ordering, partial population, and end-to-end propagation. |
| CHANGELOG.md | Documents the new fields under Added. |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to set the initial pseudo-TTY console size for job-exec jobs via new console-height and console-width configuration fields. The changes include updates to the ExecJob and ExecConfig structures, the Docker adapter to pass these dimensions to the Docker API, and a new test suite to ensure correct behavior and backward compatibility. I have no feedback to provide.
Code reviewer "Important" + test engineer "High" findings applied. Test engineer High: the adapter-level SDK boundary at core/adapters/docker/exec.go:64 (the `ConsoleSize: config.ConsoleSize` line in the containertypes.ExecOptions struct literal) had no test coverage. A refactor that dropped it would silently break #235 and all 5 core-level tests would still pass because they only exercise the mock provider. Added two adapter tests using an httptest server to capture the SDK request body: - TestExecServiceAdapter_Create_PropagatesConsoleSize asserts the ConsoleSize value reaches the daemon's /containers/{id}/exec POST body with the correct [height, width] order. - TestExecServiceAdapter_Create_OmitsConsoleSizeWhenNil pins the JSON `omitempty` contract: a nil ConsoleSize must NOT appear in the request body, otherwise Docker would treat {0,0} as an explicit-default override. Test engineer Medium / code reviewer Suggestion: pin the TTY-vs-ConsoleSize ownership contract — Ofelia owns the wire-the- value layer; the Docker daemon owns the "honor it only when TTY is true" layer. New test TestExecJob_RunWithStreams_PropagatesConsoleSizeEvenWhenTTYFalse ensures a future well-meaning patch can't add `if j.TTY { ... }` and silently swallow operator intent for non-TTY jobs. Code reviewer Important: documentation gap. docs/jobs.md and docs/CONFIGURATION.md didn't mention the new console-height / console-width fields, so INI/labels operators couldn't discover them. - docs/jobs.md: added two entries next to the existing `tty` parameter in the `exec` section. Each documents the TTY-gating, Docker API version floor (v1.42+), and the independent-dimension contract. - docs/CONFIGURATION.md: added one-line examples in the job-exec INI block with inline comments naming the issue. Code reviewer Suggestion: clarified the consoleSize() doc-comment to note that partial values like {40, 0} are intentionally forwarded (daemon honors the 40 and uses its default for the zero dimension); only "both zero" collapses to nil. Deferred: - TTY-mismatch validation warning (code reviewer Suggestion). The field godoc already calls out the daemon's silent-ignore behavior; a load-time warning would touch config validator and is scope creep for a small Docker-field addition. - Upper-bound sanity check (code reviewer Suggestion). Docker validates daemon-side; uint type already prevents negatives. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
Multi-axis review summary (code-reviewer + test-engineer + Copilot)All findings applied across one fixup commit ( Applied — Important / High
Applied — Medium
Applied — Doc/Suggestion
Deferred — with rationale on threads
Final stats: 7 tests (5 core + 2 adapter), 2 doc files updated, CHANGELOG entry. All 2 inline AI threads replied + resolved. |
Operators can now configure the initial pseudo-TTY console size for job-exec jobs via two new fields: [job-exec "my-job"] tty = true console-height = 24 console-width = 80 Useful for jobs that render TUIs, tables, or formatted text — applications that expect a specific terminal geometry (htop, vim, formatted reports) now render correctly instead of relying on Docker's default console size. Closes #235. Implementation: - New ExecJob.ConsoleHeight + ConsoleWidth (uint, mapstructure-tagged) hash:"true" so config changes trigger reload. - New ExecJob.consoleSize() helper returns *[2]uint{height, width} or nil when both dimensions are zero — matching the Docker SDK contract (nil = "use Docker's default", any populated value is sent verbatim). - New domain.ExecConfig.ConsoleSize *[2]uint plumbing. - Adapter wires through to containertypes.ExecOptions.ConsoleSize (Docker SDK 28.5.2, field added in API v1.42 / Docker 20.10). Tests (5 new, all in core/execjob_console_size_test.go): - ConsoleSize_NilWhenUnset: pre-#235 default behavior preserved when both dimensions are zero. - ConsoleSize_HeightWidthOrder: pins the [height, width] order Docker expects — swapping would produce silently-wrong TUI geometry. - ConsoleSize_PartialPopulates: setting only one dimension still produces a non-nil ConsoleSize (partial config is operator- meaningful). - RunWithStreams_PropagatesConsoleSize: end-to-end via mock provider asserting ConsoleSize arrives at domain.ExecConfig. - RunWithStreams_DefaultsToNilConsoleSize: a job without the new fields still produces nil ConsoleSize, preserving the legacy "Docker default" behavior. Backward compat: existing configs without console-height/console-width behave exactly as before. Only honored when tty = true; otherwise the Docker daemon silently ignores the size (verified by Docker API spec). CHANGELOG entry under [Unreleased] ### Added. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Code reviewer "Important" + test engineer "High" findings applied. Test engineer High: the adapter-level SDK boundary at core/adapters/docker/exec.go:64 (the `ConsoleSize: config.ConsoleSize` line in the containertypes.ExecOptions struct literal) had no test coverage. A refactor that dropped it would silently break #235 and all 5 core-level tests would still pass because they only exercise the mock provider. Added two adapter tests using an httptest server to capture the SDK request body: - TestExecServiceAdapter_Create_PropagatesConsoleSize asserts the ConsoleSize value reaches the daemon's /containers/{id}/exec POST body with the correct [height, width] order. - TestExecServiceAdapter_Create_OmitsConsoleSizeWhenNil pins the JSON `omitempty` contract: a nil ConsoleSize must NOT appear in the request body, otherwise Docker would treat {0,0} as an explicit-default override. Test engineer Medium / code reviewer Suggestion: pin the TTY-vs-ConsoleSize ownership contract — Ofelia owns the wire-the- value layer; the Docker daemon owns the "honor it only when TTY is true" layer. New test TestExecJob_RunWithStreams_PropagatesConsoleSizeEvenWhenTTYFalse ensures a future well-meaning patch can't add `if j.TTY { ... }` and silently swallow operator intent for non-TTY jobs. Code reviewer Important: documentation gap. docs/jobs.md and docs/CONFIGURATION.md didn't mention the new console-height / console-width fields, so INI/labels operators couldn't discover them. - docs/jobs.md: added two entries next to the existing `tty` parameter in the `exec` section. Each documents the TTY-gating, Docker API version floor (v1.42+), and the independent-dimension contract. - docs/CONFIGURATION.md: added one-line examples in the job-exec INI block with inline comments naming the issue. Code reviewer Suggestion: clarified the consoleSize() doc-comment to note that partial values like {40, 0} are intentionally forwarded (daemon honors the 40 and uses its default for the zero dimension); only "both zero" collapses to nil. Deferred: - TTY-mismatch validation warning (code reviewer Suggestion). The field godoc already calls out the daemon's silent-ignore behavior; a load-time warning would touch config validator and is scope creep for a small Docker-field addition. - Upper-bound sanity check (code reviewer Suggestion). Docker validates daemon-side; uint type already prevents negatives. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
86bc573 to
eaf7527
Compare
|
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.



Summary
Operators can now configure the initial pseudo-TTY console size for
job-execjobs via two new fields:```ini
[job-exec "my-job"]
tty = true
console-height = 24
console-width = 80
```
Useful for jobs that render TUIs, tables, or formatted text — applications that expect a specific terminal geometry (`htop`, `vim`, formatted reports) now render correctly instead of relying on Docker's default console size.
Closes #235.
Backward compatibility
Tests (5 new)
Test plan