Wait for rollout #260
Merged
Wait for rollout #260
Conversation
This file contains 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
Starefossen
force-pushed
the
deploy-rollout-wait
branch
from
1c33eff to
db4c23b
Compare
There was a problem hiding this comment.
PR Overview
This pull request enhances the Unleash controllers by adding a deployment readiness check, refactoring error and status handling, and updating the test suite to cover new functionalities.
- Introduces a waitForDeployment function and associated tests for deployment rollout readiness.
- Refactors error handling in the Reconcile function with more descriptive error variable names.
- Consolidates code changes across controllers and utilities to support the enhanced deployment and network policy management.
Reviewed Changes
| File | Description |
|---|---|
| pkg/utils/k8s_test.go | Added tests for the new DeploymentIsReady utility function. |
| controllers/unleash_controller_test.go | Introduced helper functions and test cases for faking deployment statuses. |
| pkg/utils/k8s.go | Added the DeploymentIsReady utility function. |
| controllers/unleash_controller.go | Updated the reconcile loop to include a deployment rollout wait and refined error handling. |
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
controllers/unleash_controller.go:213
- The variable 'statsusErr' appears to be a typo; consider renaming it to 'statusErr' for clarity and consistency with its intended meaning.
if statsusErr := r.updateStatusReconcileFailed(ctx, unleash, err, "Failed to reconcile Secrets"); statsusErr != nil {
bdd98e5 to
a17c7d8
Compare
There was a problem hiding this comment.
Pull Request Overview
This pull request adds a deployment readiness check to the Unleash controller along with error handling refactors and new test cases for deployment status. Key changes include:
- Introducing a waitForDeployment function with a new timeout and requeueAfter constant.
- Refactoring the reconcile flow to log and update error messages consistently.
- Expanding the test suite with new deployment readiness scenarios.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/utils/k8s.go | Added/updated DeploymentIsReady function with added documentation comment. |
| pkg/utils/k8s_test.go | Added tests validating the behavior of DeploymentIsReady under different conditions. |
| controllers/unleash_controller.go | Reordered and enhanced reconcile steps by introducing waitForDeployment and updating errors. |
| controllers/unleash_controller_test.go | Added utility functions and test cases to simulate deployment status and readiness changes. |
Comments suppressed due to low confidence (3)
controllers/unleash_controller.go:235
- [nitpick] Ensure that the span event messages and the corresponding error update messages remain consistent as the reconcile order has changed. This will improve traceability and debugging clarity.
span.AddEvent("Reconciling NetworkPolicy")
controllers/unleash_controller.go:312
- [nitpick] The error handling block for waitForDeployment repeats the status update call and error return, leading to potential redundancy. Consider refactoring to call updateStatusReconcileFailed once and then return the error.
if err = r.waitForDeployment(ctx, deploymentTimeout, req.NamespacedName); err != nil {
controllers/unleash_controller_test.go:198
- [nitpick] Similar blocks of code for simulating deployment status (failed/available) appear multiple times. Consider extracting a helper function to reduce duplication and improve maintainability in tests.
setDeploymentStatusFailed(createdDeployment)
8a7a3f2 to
d3fb90c
Compare
Starefossen
force-pushed
the
deploy-rollout-wait
branch
from
efd3dd5 to
9f15805
Compare
There was a problem hiding this comment.
Pull Request Overview
This pull request enhances the Unleash reconciler by adding a deployment readiness check, refactoring error handling, and updating associated tests and configuration files. Key changes include:
- Introducing a rollout wait mechanism with a new waitForDeployment function.
- Refactoring reconciliation order and error handling in the controller.
- Updating test suites, YAML charts, and utilities for deployment and ApiToken status.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| charts/unleasherator/templates/tests/test-unleash-v5.yaml | New YAML test manifest for Unleash instance connection validation |
| charts/unleasherator/ci/k3s-values.yaml | Updated configuration values including network policy and OTLP endpoint settings |
| pkg/utils/k8s_test.go | Added tests for Deployment readiness utility |
| controllers/unleash_controller_test.go | Added helper functions for faking Deployment status in tests |
| controllers/unleash_controller.go | Refactored reconciliation order; introduced rollout waiting via waitForDeployment |
| charts/unleasherator/ci/gcp-values.yaml | Adjusted API server CIDR and allowed namespaces |
| pkg/utils/k8s.go | Added DeploymentIsReady utility function |
| .github/workflows/chart.yaml | Updated helm command with extended timeout arguments |
| config/samples/unleash_v1_apitoken.yaml | Fixed typo in the sample ApiToken manifest |
| controllers/apitoken_controller.go | Renamed constant for requeue duration and updated status handling |
Comments suppressed due to low confidence (4)
charts/unleasherator/ci/k3s-values.yaml:18
- Review the indentation and structure of the 'otlpServiceEndpoint' configuration to ensure it adheres to valid YAML schema.
otlpServiceEndpoint:
controllers/unleash_controller_test.go:74
- [nitpick] Consider increasing the deploymentTimeout duration in tests to avoid potential flakiness in environments with slower response times.
deploymentTimeout = time.Second * 1
controllers/unleash_controller.go:235
- [nitpick] Ensure that the reordering of reconciliation steps (NetworkPolicy before Deployment) is intentional, as it differs from the previous sequence.
span.AddEvent("Reconciling NetworkPolicy")
config/samples/unleash_v1_apitoken.yaml:11
- Correct the spelling of 'unleasshInstance' to 'unleashInstance'.
unleasshInstance:
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This pull request introduces several enhancements and fixes to the
UnleashReconcilerin thecontrollers/unleash_controller.gofile. The most significant changes include adding a deployment readiness check, refactoring error handling, and updating the test suite to cover new functionalities.Enhancements and Fixes:
Deployment Readiness Check:
deploymentTimeoutand a functionwaitForDeploymentto ensure the deployment rollout is complete before proceeding. (controllers/unleash_controller.go) [1] [2] [3]getDeployment,setDeploymentStatusFailed, andsetDeploymentStatusAvailableto facilitate deployment status checks in tests. (controllers/unleash_controller_test.go) [1] [2] [3]Error Handling:
Reconcilefunction to use a more consistent variable namestatusErrfor status update errors. (controllers/unleash_controller.go) [1] [2] [3] [4]Test Suite Updates:
controllers/unleash_controller_test.go) [1] [2]TestDeploymentIsReadyfunction to validate the deployment readiness utility. (pkg/utils/k8s_test.go)Utility Enhancements:
DeploymentIsReadyfunction to theutilspackage for checking deployment readiness. (pkg/utils/k8s.go)Imports:
controllers/unleash_controller.goandpkg/utils/k8s.go. [1] [2] [3] [4]Close #193