You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Follow-ups from the CodeRabbit review on #459. These are test additions and refactors rather than defects, so they were split out to keep that PR to correctness fixes, comment corrections, and small changes. Each was verified as still applicable at 4afdc73.
New specs
setJobPackage stamps both the Job and the pod template — job_builder.go notes the template copy is load-bearing (pod_controller.go reads the package annotation off the child pod, which inherits only template metadata). No spec asserts both copies exist, so a regression is silent. (job_builder_test.go ~257)
The terminating-Job guard — JobReconcile returns early when job.DeletionTimestamp != nil, preventing a completion being recorded onto node state a sweep just reset. Uncovered. The fake client sets DeletionTimestamp only when a finalizer is present. (job_controller_test.go ~307)
Post-terminal Job retention in e2e — assert_jobs.yaml runs before completion, so it never observes ttlSecondsAfterFinished, which the controller sets at completion. Do not assert a literal Job name; the builder derives it from a hashed pod name. (simple-nodewright/chainsaw-test.yaml ~91)
Refactors
Convert TestTailAndSanitize / TestGetPodLogTail to Ginkgo/Gomega — the package owns a Ginkgo suite (dal_suite_test.go), so these stdlib t.Run cases run outside its reporting and randomization. CLAUDE.md calls for Ginkgo over t.Run. Note one case was added to TestTailAndSanitize in feat(operator): migrate package execution from raw Pods to Kubernetes Jobs (#223) #459 (the post-sanitize byte cap) and should carry over. (dal_test.go ~210)
Share one options helper in node_state_merge_test.go — the saveNodeChanges Describe repeats a SkyhookOperatorOptions literal twice while saveNodeChanges conflict retry already defines an equivalent opts() helper; move it to package scope. (~212)
Not included, deliberately
Two review comments asked for Apache-2.0 headers on Markdown files and on chart/templates/*.yaml. The license tooling covers *.go, *.py, *.sh, *.yaml, *.yml, Dockerfile and runs from operator/, so neither Markdown nor chart/ is in scope — and no tracked file of either kind carries a header today. Adding one would make those files the outliers.
Follow-ups from the CodeRabbit review on #459. These are test additions and refactors rather than defects, so they were split out to keep that PR to correctness fixes, comment corrections, and small changes. Each was verified as still applicable at 4afdc73.
New specs
setJobPackagestamps both the Job and the pod template —job_builder.gonotes the template copy is load-bearing (pod_controller.goreads the package annotation off the child pod, which inherits only template metadata). No spec asserts both copies exist, so a regression is silent. (job_builder_test.go~257)JobReconcilereturns early whenjob.DeletionTimestamp != nil, preventing a completion being recorded onto node state a sweep just reset. Uncovered. The fake client setsDeletionTimestamponly when a finalizer is present. (job_controller_test.go~307)assert_jobs.yamlruns before completion, so it never observesttlSecondsAfterFinished, which the controller sets at completion. Do not assert a literal Job name; the builder derives it from a hashed pod name. (simple-nodewright/chainsaw-test.yaml~91)Refactors
TestTailAndSanitize/TestGetPodLogTailto Ginkgo/Gomega — the package owns a Ginkgo suite (dal_suite_test.go), so these stdlibt.Runcases run outside its reporting and randomization.CLAUDE.mdcalls for Ginkgo overt.Run. Note one case was added toTestTailAndSanitizein feat(operator): migrate package execution from raw Pods to Kubernetes Jobs (#223) #459 (the post-sanitize byte cap) and should carry over. (dal_test.go~210)node_state_merge_test.go— thesaveNodeChangesDescribe repeats aSkyhookOperatorOptionsliteral twice whilesaveNodeChanges conflict retryalready defines an equivalentopts()helper; move it to package scope. (~212)Not included, deliberately
Two review comments asked for Apache-2.0 headers on Markdown files and on
chart/templates/*.yaml. The license tooling covers*.go,*.py,*.sh,*.yaml,*.yml,Dockerfileand runs fromoperator/, so neither Markdown norchart/is in scope — and no tracked file of either kind carries a header today. Adding one would make those files the outliers.