-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update condition lib to use v2 #61
Update condition lib to use v2 #61
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick doubt, is OLM still going to support v1 condition CRD's for a release and then deprecate (eventually remove) later? If thats the case, I suppose this condition lib should also be versioned in a way to support both the condition CRD versions till OLM removes v1 Conditions
. Or is it not necessary?
cc: @estroz @jmrodri
OperatorCondition V1 does not handle race conditions so it is broken. We do not want anyone using v1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Pull Request Test Coverage Report for Build 1004777565
💛 - Coveralls |
So clearly there is not enough test coverage somewhere that resulted in this race condition not being caught. Has that coverage been implemented somewhere? Are more tests needed here? |
ObjectMeta: metav1.ObjectMeta{Name: "operator-condition-test", Namespace: ns}, | ||
Status: apiv1.OperatorConditionStatus{ | ||
Spec: apiv2.OperatorConditionSpec{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a non-blocker, but it would be helpful to have conditionType
declared in v2 too. Its odd that for someone using this library, they would have to use conditionType
from v1
and refer to OperatorCondition api from v2
. Example - here.
If not the other option is - to remove the conditionType
itself and let users specify a string directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in this release: https://github.com/operator-framework/api/releases/tag/v0.9.1
@estroz this should be handled in the OLM PR operator-framework/operator-lifecycle-manager#2160 |
f89aaba
to
180e696
Compare
180e696
to
3ae2713
Compare
/lgtm |
Reposting from operator-framework/api#119 for visibility:
|
I don't think we will have another significant changes anytime soon. Any other changes are probably on OLM side, not on the CRD side but I'm fine with keeping it |
Resolved in operator-framework/api#119 (comment). v2 it is. |
The v2 OperatorCondition CRD was introduced to the operator-framework/api repository. This change updates the condition library to consume the new changes.
3ae2713
to
cc9d1af
Compare
@varshaprasad96 can we get eyes on this pr? |
@@ -84,7 +84,7 @@ func (c *condition) Get(ctx context.Context) (*metav1.Condition, error) { | |||
|
|||
// Set implements conditions.Set | |||
func (c *condition) Set(ctx context.Context, status metav1.ConditionStatus, option ...Option) error { | |||
operatorCond := &apiv1.OperatorCondition{} | |||
operatorCond := &apiv2.OperatorCondition{} | |||
err := c.client.Get(ctx, c.namespacedName, operatorCond) | |||
if err != nil { | |||
return ErrNoOperatorCondition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider returning the original error, or at least wrap it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I support this change, but recommend doing so in a separate commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #65
@@ -69,12 +69,12 @@ func NewCondition(cl client.Client, condType apiv1.ConditionType) (Condition, er | |||
|
|||
// Get implements conditions.Get | |||
func (c *condition) Get(ctx context.Context) (*metav1.Condition, error) { | |||
operatorCond := &apiv1.OperatorCondition{} | |||
operatorCond := &apiv2.OperatorCondition{} | |||
err := c.client.Get(ctx, c.namespacedName, operatorCond) | |||
if err != nil { | |||
return nil, ErrNoOperatorCondition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider returning the original error, or at least wrap it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I support this change, but recommend doing so in a separate commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #65
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Thanks @awgreene !
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: varshaprasad96 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 |
The v2 OperatorCondition CRD was introduced to the operator-framework/api
repository. This change updates the condition library to consume the new
changes.