feat: add livenessProbe to Package Manager#137
Conversation
|
Claude finished @stevenolen's task —— View job Review Complete
Submitted a non-blocking review. The change looks good — follows existing patterns and uses appropriate upstream defaults. Left two minor suggestions around field consistency and test coverage. |
There was a problem hiding this comment.
LGTM — straightforward addition that mirrors the existing readinessProbe structure and uses sensible values from upstream defaults.
Minor observations:
-
Consistency with readinessProbe fields (
package_manager.go:588-599): The readinessProbe explicitly setsSuccessThreshold: 1andTerminationGracePeriodSeconds: nil. While those are defaults for liveness probes too, including them would keep the two definitions visually symmetric and make it clear nothing was accidentally omitted. Not a blocker. -
No unit test coverage for the new probe: The existing
package_manager_controller_test.godoesn't assert on probes. The flightdeck controller tests do verify their livenessProbe (flightdeck_test.go:197-200). Consider adding a similar assertion for PPM to catch accidental regressions. Also not a blocker for this PR given the test plan includes manual validation.
Overall: clean change, follows existing patterns, no correctness or security concerns.
# [1.25.0](v1.24.1...v1.25.0) (2026-05-12) ### Features * add livenessProbe to Package Manager ([#137](#137)) ([ac44f4c](ac44f4c))
Summary
/__ping__livenessProbe to the Package Manager container, alongside the existing readinessProbe.httpport to stay consistent with the readinessProbe.Test plan
go build ./...