Rework Onboarding#307
Open
fwiesel wants to merge 2 commits into
Open
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR switches onboarding condition updates to Kubernetes Server-Side Apply, adds SSA condition utilities, and updates onboarding controller flows and tests to use the new status-writing path. ChangesOnboarding Controller SSA Refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/utils/conditions_test.go`:
- Around line 30-31: The helper function ptr[T any](v T) *T in the tests is
unused and causing golangci-lint failures; remove the entire ptr function
declaration (symbol "ptr") from internal/utils/conditions_test.go so it is no
longer compiled, then run the test/lint to confirm the error is gone.
In `@internal/utils/conditions.go`:
- Around line 61-63: The loop that compares condition types dereferences
newCondition.Type without checking it, which can panic; update the loop in the
function using variables conditions, newCondition and existing so you first
check that newCondition.Type != nil (and optionally (*conditions)[i].Type !=
nil) before comparing *(*conditions)[i].Type == *newCondition.Type, returning or
skipping the comparison when newCondition.Type is nil to avoid the dereference.
- Around line 33-39: The condition conversion currently builds
k8sacmetav1.ConditionApplyConfiguration without copying ObservedGeneration, so
update the struct literal that creates k8sacmetav1.ConditionApplyConfiguration
(where result[i] = k8sacmetav1.ConditionApplyConfiguration{...}) to include
ObservedGeneration: &c.ObservedGeneration; also add/adjust the test that
constructs a metav1.Condition to assert the ObservedGeneration is preserved
after conversion so the omission is covered by tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f6306d2-89fd-46ac-b2d2-711ee3393c78
📒 Files selected for processing (4)
internal/controller/onboarding_controller.gointernal/controller/onboarding_controller_test.gointernal/utils/conditions.gointernal/utils/conditions_test.go
|
This PR is stale because it has been open for 45 days with no activity. |
fwiesel
force-pushed
the
rework-onboarding
branch
from
311d8e2 to
9980b46
Compare
fwiesel
force-pushed
the
rework-onboarding
branch
from
9980b46 to
ef9b6f1
Compare
fwiesel
force-pushed
the
rework-onboarding
branch
2 times, most recently
from
f757216 to
b6f16a2
Compare
fwiesel
force-pushed
the
rework-onboarding
branch
from
0f495aa to
aeb6791
Compare
When the test instance creation in the onboarding smoke test fails, the controller used to return a bare error from Reconcile and never updated the Onboarding condition. Operators saw a stale message frozen for days while the loop kept silently retrying — combined with the SSA condition helper preserving LastTransitionTime when Status does not change, the resource looked stuck even though reconciliation was healthy. This change: - In smokeTest, when createOrGetTestServer fails, set the Onboarding condition to Testing with the current error message and requeue with defaultWaitTime instead of returning the error. This makes each retry observable in the resource itself and matches the style used by every other smokeTest error path. - Include the failed server's UUID in the ERROR-state, console-output-failure, timeout, and terminate-failure messages. A fresh attempt allocates a new instance UUID, so each retry yields a different message even when Nova keeps returning the same fault text preventing SetApplyConfigurationStatusCondition from treating successive failures as no-op updates. - Tests: add Ginkgo specs covering POST /servers returning 500 and a server in ERROR state, asserting the condition message, the requeue, the absence of a returned error, and (for ERROR) that the delete is issued and the UUID is in the message.
Covers ConditionFromStatus (including ObservedGeneration preservation) and SetApplyConfigurationStatusCondition (append, update, status-change, LastTransitionTime preservation, no-op detection, multi-condition isolation).
fwiesel
force-pushed
the
rework-onboarding
branch
from
aeb6791 to
40cd7f2
Compare
|
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
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
1 participant
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.
Refines the onboarding controller on top of the SSA migration (
ssabranch, #330).Changes:
createOrGetTestServerfails, the error is surfaced in theOnboardingcondition instead of returning a bare error. This makes each retry visible to operators; previously the condition message was frozen even though reconciliation was healthy.SetApplyConfigurationStatusConditionfrom treating successive failures as no-ops.Depends on:
ssa(#330).