Repository navigation
fix(chart): bound CronJob names to the API limit - #670
mrbobbytables wants to merge 2 commits into
Conversation
Use a shared bounded CronJob name helper in the manifest and install notes. Pin long release names, the length boundary, short-name compatibility, and fullname override truncation. Hive-Run: #667 Hive-Plan: cronjob-name-limit Hive-Spec: issue-667#recommendation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: mrbobbytables <mrbobbytables@users.noreply.github.com>
There was a problem hiding this comment.
The fix itself is correct — I verified the arithmetic against the tree. prow-github-app.cronjobName (_helpers.tpl:19-21) truncates fullname to 47 and appends -jobs, giving a 52-char ceiling, which is exactly the 63-char object-name limit minus the 11 characters the Job controller appends for scheduled Jobs. The boundary test (31-char release + -prow-github-app = 47, +-jobs = 52) and the trimSuffix "-" override case both check out, and NOTES.txt:15 now names the same resource the manifest creates.
The red build-test check is this PR's own diff, though: eslint's yaml/plain-scalar rule rejects the quoted '-jobs$' patterns in charts/prow-github-app/tests/naming_test.yaml:22 and :28:
charts/prow-github-app/tests/naming_test.yaml
22:52 error Must use plain style scalar yaml/plain-scalar
28:52 error Must use plain style scalar yaml/plain-scalar
Writing them unquoted (pattern: -jobs$) fixes it — npx eslint --fix on the file does it automatically. The two long regexes on lines 21 and 27 are fine as-is: they contain {/, so plain style is impossible inside the flow maps and the rule doesn't fire on them.
No conflict with #669: that PR pins RELEASE-NAME-bot-jobs, which at 21 characters is untouched by the 47-char truncation here.
One lint fix away from green; otherwise looks correct to me.
— hive: agent=reviewer backend=copilot model=claude-fable-5 copilot=1.0.88
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks for catching this. I changed both 🐝 Hive Agent: |
|
All CI checks are green after b754713, but the DCO check is still red: that commit has no Reviewed the test cases against the helper: the 52-character case (31 + 🐝 Hive Agent: — hive: agent=quality backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
There was a problem hiding this comment.
Read the chart tree at head b754713, not just the diff. The fix is correct; one process blocker remains.
Correctness — verified:
_helpers.tpl:19-21:fullname | trunc 47 | trimSuffix "-"+-jobs(5 chars) caps the name at 52, exactly the CronJob limit (the Job controller appends an 11-char suffix under the 63-char DNS label). The 31-char-release boundary case intests/naming_test.yaml:12-16lands precisely on 52 (31 + "-prow-github-app" (16) + "-jobs" (5)).- Names ≤52 today render byte-identical (trunc 47 only bites when fullname > 47; any fullname of 48+ already produced a >52-char name the API server rejected), so no working deployment is renamed on upgrade.
cronjob.yaml:5andNOTES.txt:15are the only two places that print this resource name (grep -rn -- '-jobs' charts/— theprow-jobscontainer name atcronjob.yaml:35is unrelated), and both now use the shared helper, so the notes name the resource actually created.- The hyphen-trim case (
naming_test.yaml:28-33) is right: char 47 of the override is-,trimSuffixyields the asserted 51-char name.
This matches the recommendation in #667 verbatim, including the five test cases. No scope creep.
Blocker: the head commit b754713 ("use plain scalar for job suffix patterns") has no Signed-off-by trailer, so the DCO check is ACTION_REQUIRED and the PR cannot merge — despite the checklist claiming sign-off (the first commit b9b5af5 is signed off). Rebase with git commit --amend -s (or squash into the signed-off commit) and push.
Code-wise this looks correct to me; once the sign-off lands it should be mergeable.
— hive: agent=reviewer backend=copilot model=claude-fable-5 copilot=1.0.88
|
/kind cleanup |
There was a problem hiding this comment.
Test-quality check at head b754713: the new naming_test.yaml cases are genuine regression tests.
untt charts/prow-github-appat head: 46/46 pass (5 suites).- Same test files with
charts/prow-github-app/templatesswapped back toorigin/main: 3 failed, 43 passed — exactly the three cases the description says fail before the fix (keeps a long release's CronJob name within the API limit,bounds a maximum-length release name that already contains the chart name,trims a hyphen at the truncation boundary of a fullname override). The two compatibility cases (short release, 52-char boundary) pass on both trees, as intended.
Nothing further from the quality lane; the DCO sign-off on b754713 noted above is the only remaining blocker.
— hive: agent=quality backend=copilot model=claude-fable-5.1
Description
Fixes #667.
Bound the CronJob name to 52 characters with a shared helper, reserving room for
-jobsand trimming a hyphen at the truncation boundary. Use the same helper in install notes so they name the actual resource. Existing names within the limit are unchanged.Testing
helm unittest charts/prow-github-app: 46 tests passed.helm lint --strictwith each of the single-replica and HA CI values files.helm install --dry-run=clientwith the long release name: NOTES and manifest name agree.git diff --check.The full
ct lintcommand was not run: chart-testing is absent and the environment's command allowlist denied its release download. Both chart configurations passed Helm lint. No cluster install was run.Checklist
npm run all/npm run pack: not run; chart-only change, no application source or bundle changes.— hive: backend=copilot model=claude-opus-5.5
🐝 Hive Agent:
contributor| SHA:b9b5af5