Add Configured condition to all controllers#426
Open
adamtrizuljak-sap wants to merge 7 commits into
Open
Conversation
Comment on lines
-446
to
+448
| Type: v1alpha1.ReadyCondition, | ||
| Type: v1alpha1.ConfiguredCondition, |
Collaborator
Author
There was a problem hiding this comment.
@nikatza I'm not sure if this is entirely correct and I'd like to discuss it with you. It seems like we should set both conditions here, not just replace the Ready condition with Configured condition. Same goes for the other files where this was done.
Contributor
There was a problem hiding this comment.
I think this is correct with only setting the "Configured" Condition. The reconcileVRF func is called by reconcile. If the former sets the "Configured" condition, the later still calls RecomputeReady in it's defer call when returning. So the Ready condition would be set according on the "Configured" condition, which is what we want.
Collaborator
Author
There was a problem hiding this comment.
Yes that seems to be right. We set the Configured condition explicitly here and setting the Ready condition is taken care off by the deferred call to conditions.RecomputeReady().
felix-kaestner
left a comment
felix-kaestner
left a comment
Contributor
There was a problem hiding this comment.
As we are adding a new condition to these resources, should we also add a printcolumn like so?
diff --git a/api/core/v1alpha1/acl_types.go b/api/core/v1alpha1/acl_types.go
index 6501f362..a7a01b70 100644
--- a/api/core/v1alpha1/acl_types.go
+++ b/api/core/v1alpha1/acl_types.go
@@ -122,6 +122,7 @@ type AccessControlListStatus struct {
// +kubebuilder:printcolumn:name="Device",type=string,JSONPath=`.spec.deviceRef.name`
// +kubebuilder:printcolumn:name="Entries",type=string,JSONPath=`.status.entriesSummary`,priority=1
// +kubebuilder:printcolumn:name="Ready",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].status`
+// +kubebuilder:printcolumn:name="Configured",type=string,JSONPath=`.status.conditions[?(@.type=="Configured")].status`,priority=1
// +kubebuilder:printcolumn:name="Paused",type=string,JSONPath=`.status.conditions[?(@.type=="Paused")].status`,priority=1
// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"
adamtrizuljak-sap
force-pushed
the
feat/configured-condition
branch
from
65211b0 to
4b9f4ad
Compare
Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
adamtrizuljak-sap
force-pushed
the
feat/configured-condition
branch
from
032b437 to
8d76d7d
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
|
nikatza
reviewed
Jul 6, 2026
|
|
||
| orig := obj.DeepCopy() | ||
| if conditions.InitializeConditions(obj, v1alpha1.ReadyCondition) { | ||
| if conditions.InitializeConditions(obj, v1alpha1.ReadyCondition, v1alpha1.ConfiguredCondition) { |
Contributor
There was a problem hiding this comment.
Do you think it would make sense to include/refactor the dhcprelay_controller_test.go and align with the other tests style?
Contributor
|
@adamtrizuljak-sap We merged the Likewise, in this PR we only tackle Thanks a lot for your contribution! 🔥 |
Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
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
3 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.
https://github.wdf.sap.corp/sap-cloud-infrastructure/neutron-issues/issues/361
ConfiguredConditionconditions.RecomputeReady()to ensure the Ready condition is evaluated and set at the end of the reconcile loopcond := conditions.FromError(err)already returns the Configured condition, so we just remove the next line that was overriding it with the Ready conditionTodo
ReadyConditiontoConfiguredCondition- I suspect both conditions should be set