Draft
Conversation
jlegrone
force-pushed
the
jlegrone/add-deployment-finalizer
branch
from
e121567 to
fbc3949
Compare
jlegrone
commented
Jul 29, 2025
jlegrone
force-pushed
the
jlegrone/add-deployment-finalizer
branch
from
9617e3f to
4e13b89
Compare
jlegrone
commented
Jul 30, 2025
jlegrone
force-pushed
the
jlegrone/add-deployment-finalizer
branch
2 times, most recently
from
a484510 to
b6d8d81
Compare
jlegrone
added a commit
that referenced
this pull request
Aug 25, 2025
Previously the test was manually adding finalizers using controllerutil.AddFinalizer instead of testing the actual controller logic. Now the test calls the controller's Reconcile method to properly exercise the finalizer addition logic as intended. Addresses: #97 (comment)
jlegrone
commented
Aug 25, 2025
jlegrone
added a commit
that referenced
this pull request
Aug 25, 2025
Changed the exported constant TemporalWorkerDeploymentFinalizer to temporalWorkerDeploymentFinalizer (lowercase) to make it package-private since it's only used within the controller package. Addresses: #97 (comment)
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds a deployment finalizer to the TemporalWorkerDeployment controller to prevent accidental deletion of deployment resources that would be unrecoverable. The finalizer ensures controlled deletion following the Kubernetes finalizer pattern, preventing unrecoverable state loss when deployments are accidentally deleted.
Key changes:
- Added finalizer management in the controller reconciliation loop
- Implemented proper cleanup logic for managed deployment resources
- Added comprehensive unit tests covering finalizer behavior and edge cases
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/controller/worker_controller.go | Core finalizer implementation with deletion handling and resource cleanup logic |
| internal/controller/finalizer_test.go | Comprehensive unit tests for finalizer behavior, cleanup, and edge cases |
| internal/tests/internal/integration_test.go | Integration test validating deployment deletion protection behavior |
| internal/testhelpers/make.go | Added test utilities for scheme setup and fake client creation |
| internal/planner/planner.go | Minor code cleanup removing unnecessary else clause |
| internal/k8s/deployments.go | Updated comment explaining finalizer management approach |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
jlegrone
added a commit
that referenced
this pull request
Aug 26, 2025
- Add detailed documentation comments for hardcoded timeout values explaining the rationale behind the chosen durations (2 minutes cleanup timeout and 5 second poll interval) - Change field selector fallback logging from Info to debug level (V(1)) to reduce noise during normal operation, especially in test environments Addresses: - #97 (comment) - #97 (comment)
- Added testDeploymentDeletionProtection test to verify deployment resources can only be deleted by the controller - Test validates proper owner references with BlockOwnerDeletion=true - Verifies controller recreates deployments if directly deleted - Confirms controller properly cleans up deployments when TWD is deleted - Replaced logging statements with proper testify assertions for better test validation and debugging
- Update GitHub Actions workflow to use go-version-file instead of hardcoded Go 1.21 - Fix revive linting error in planner.go by removing unnecessary else clause - Fix deprecated result.Requeue usage in finalizer_test.go to use RequeueAfter
- Replace stale list iteration with proper polling using fresh queries - Add timeout and context cancellation handling with configurable constants - Implement field selector optimization with backward compatibility fallback - Replace brittle time.Sleep with condition-based polling in integration tests - Add comprehensive edge case tests for context cancellation and partial cleanup failures
Previously the test was manually adding finalizers using controllerutil.AddFinalizer instead of testing the actual controller logic. Now the test calls the controller's Reconcile method to properly exercise the finalizer addition logic as intended. Addresses: #97 (comment)
Changed the exported constant TemporalWorkerDeploymentFinalizer to temporalWorkerDeploymentFinalizer (lowercase) to make it package-private since it's only used within the controller package. Addresses: #97 (comment)
Removed duplicate testEnv struct declaration from integration_test.go as it was already defined in env_helpers.go. This resolves the build error that was preventing integration tests from running.
- Add detailed documentation comments for hardcoded timeout values explaining the rationale behind the chosen durations (2 minutes cleanup timeout and 5 second poll interval) - Change field selector fallback logging from Info to debug level (V(1)) to reduce noise during normal operation, especially in test environments Addresses: - #97 (comment) - #97 (comment)
jlegrone
force-pushed
the
jlegrone/add-deployment-finalizer
branch
from
83f07d5 to
0e1a946
Compare
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was changed
Added a deployment finalizer to prevent accidental deletion of deployment resources that would be unrecoverable for the controller.
Why?
The Temporal worker controller doesn't persist the original pod template anywhere other than in the live Kubernetes deployment resource. When a deployment is accidentally deleted (e.g., via
kubectl delete deployment), the controller cannot recreate it since it doesn't have the old version of the pod template saved anywhere else.This finalizer follows the Kubernetes finalizers pattern to ensure controlled deletion and prevent unrecoverable state loss.
Checklist
Closes Add Finalizer for Deployment Resources #55
How was this tested: New integration test validates finalizer behavior during deployment deletion
Any docs updates needed: No documentation updates required - this is an internal implementation detail