Add logic to set the operator's status in the NFD CR #84
Conversation
|
Hi @courtneypacheco. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/assign @ArangoGutierrez |
There was a problem hiding this comment.
Thanks @courtneypacheco! Could you rebase this on top of latest master. It would make the review far more easier (to not see the effects of merge(s))
/ok-to-test
|
@marquiz Thanks! I noticed some of my tests failed, so I'm adding fixes. I will certainly rebase and squash. :) |
7f75fcb to
4e2f6cd
Compare
Add logic that sets the operator's status in the NFD CR based on whether one or more of NFD's resources is: degraded, progressing, upgradeable, or available. Also add a "applyComponents" function to simplify readability.
4e2f6cd to
d97cfd2
Compare
|
@marquiz Just squashed commits and rebased. PTAL. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ArangoGutierrez, courtneypacheco The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| rstatus, err := r.getSecurityContextConstraintsConditions(ctx) | ||
| if rstatus.isDegraded { | ||
| return r.updateDegradedCondition(instance, err.Error(), err) | ||
|
|
There was a problem hiding this comment.
Unneeded empty line. Ditto X times below
| } else if err != nil { | ||
| return r.updateDegradedCondition(instance, conditionFailedGettingNFDScc, err) | ||
| } |
There was a problem hiding this comment.
Always first check for error, then other conditions. Ditto X times below
Also, you could streamline a bit. Suggestion:
if rstatus, err := r.getSecurityContextConstraintsConditions(ctx); err != nil {
...
} else if rstatus.isDegraded {
...
}
| if result != nil { | ||
| return *result, nil | ||
| } |
There was a problem hiding this comment.
Looks suspicious, masking the error 😦
|
|
||
| // update ports |
| "errors" | ||
| "time" | ||
|
|
||
| conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1" |
There was a problem hiding this comment.
Hmm, I'm not sure we want to depend on downstream (openshift) at least this heavily 🧐 I think we should use k8s metav1 (k8s.io/apimachinery/pkg/apis/meta/v1).
Maybe ConditionTypes is something that we could use from openshift 🤔 Or then just copy paste those here in order to break the dependency
|
@courtneypacheco: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Superseed by #88 |
|
@ArangoGutierrez: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Add logic that sets the operator's status in the NFD CR based on whether one or more of NFD's resources is: degraded, progressing, upgradeable, or available. Also add a "applyComponents" function to simplify readability.