Skip to content

Update condition lib to use v2 #61

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 6, 2021
Merged

Update condition lib to use v2 #61

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions conditions/conditions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import (
"fmt"
"os"

apiv1 "github.com/operator-framework/api/pkg/operators/v1"
apiv2 "github.com/operator-framework/api/pkg/operators/v2"
"github.com/operator-framework/operator-lib/internal/utils"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -46,7 +46,7 @@ const (
// conditionType in the OperatorCondition CR.
type condition struct {
namespacedName types.NamespacedName
condType apiv1.ConditionType
condType apiv2.ConditionType
client client.Client
}

Expand All @@ -55,7 +55,7 @@ var _ Condition = &condition{}
// NewCondition returns a new Condition interface using the provided client
// for the specified conditionType. The condition will internally fetch the namespacedName
// of the operatorConditionCRD.
func NewCondition(cl client.Client, condType apiv1.ConditionType) (Condition, error) {
func NewCondition(cl client.Client, condType apiv2.ConditionType) (Condition, error) {
objKey, err := GetNamespacedName()
if err != nil {
return nil, err
Expand All @@ -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
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #65

}
con := meta.FindStatusCondition(operatorCond.Status.Conditions, string(c.condType))
con := meta.FindStatusCondition(operatorCond.Spec.Conditions, string(c.condType))

if con == nil {
return nil, fmt.Errorf("conditionType %v not found", c.condType)
Expand All @@ -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
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #65

Expand All @@ -100,7 +100,7 @@ func (c *condition) Set(ctx context.Context, status metav1.ConditionStatus, opti
opt(newCond)
}
}
meta.SetStatusCondition(&operatorCond.Status.Conditions, *newCond)
meta.SetStatusCondition(&operatorCond.Spec.Conditions, *newCond)
err = c.client.Status().Update(ctx, operatorCond)
if err != nil {
return err
Expand Down
24 changes: 12 additions & 12 deletions conditions/conditions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
apiv1 "github.com/operator-framework/api/pkg/operators/v1"
apiv2 "github.com/operator-framework/api/pkg/operators/v2"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand All @@ -32,8 +32,8 @@ import (
)

const (
conditionFoo apiv1.ConditionType = "conditionFoo"
conditionBar apiv1.ConditionType = "conditionBar"
conditionFoo apiv2.ConditionType = "conditionFoo"
conditionBar apiv2.ConditionType = "conditionBar"
)

var _ = Describe("Condition", func() {
Expand All @@ -46,7 +46,7 @@ var _ = Describe("Condition", func() {

BeforeEach(func() {
sch := runtime.NewScheme()
err = apiv1.AddToScheme(sch)
err = apiv2.AddToScheme(sch)
Expect(err).NotTo(HaveOccurred())
cl = fake.NewClientBuilder().WithScheme(sch).Build()
})
Expand Down Expand Up @@ -75,17 +75,17 @@ var _ = Describe("Condition", func() {
})

Describe("Get/Set", func() {
var operatorCond *apiv1.OperatorCondition
var operatorCond *apiv2.OperatorCondition

objKey := types.NamespacedName{
Name: "operator-condition-test",
Namespace: ns,
}

BeforeEach(func() {
operatorCond = &apiv1.OperatorCondition{
operatorCond = &apiv2.OperatorCondition{
ObjectMeta: metav1.ObjectMeta{Name: "operator-condition-test", Namespace: ns},
Status: apiv1.OperatorConditionStatus{
Spec: apiv2.OperatorConditionSpec{
Copy link
Member

@varshaprasad96 varshaprasad96 May 26, 2021 •
edited
Loading

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.

Copy link
Member

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

Conditions: []metav1.Condition{
{
Type: string(conditionFoo),
Expand All @@ -107,7 +107,7 @@ var _ = Describe("Condition", func() {

// create a new client
sch := runtime.NewScheme()
err = apiv1.AddToScheme(sch)
err = apiv2.AddToScheme(sch)
Expect(err).NotTo(HaveOccurred())
cl = fake.NewClientBuilder().WithScheme(sch).Build()

Expand Down Expand Up @@ -171,12 +171,12 @@ var _ = Describe("Condition", func() {
Expect(err).NotTo(HaveOccurred())

By("fetching the condition from cluster")
op := &apiv1.OperatorCondition{}
op := &apiv2.OperatorCondition{}
err = cl.Get(ctx, objKey, op)
Expect(err).NotTo(HaveOccurred())

By("checking if the condition has been updated")
res := op.Status.Conditions[0]
res := op.Spec.Conditions[0]
Expect(res.Message).To(BeEquivalentTo("test"))
Expect(res.Status).To(BeEquivalentTo(metav1.ConditionFalse))
Expect(res.Reason).To(BeEquivalentTo("not_in_foo_state"))
Expand All @@ -190,12 +190,12 @@ var _ = Describe("Condition", func() {
Expect(err).NotTo(HaveOccurred())

By("fetching the condition from cluster")
op := &apiv1.OperatorCondition{}
op := &apiv2.OperatorCondition{}
err = cl.Get(ctx, objKey, op)
Expect(err).NotTo(HaveOccurred())

By("checking if the condition has been updated")
res := op.Status.Conditions
res := op.Spec.Conditions
Expect(len(res)).To(BeEquivalentTo(2))
Expect(meta.IsStatusConditionTrue(res, string(conditionBar))).To(BeTrue())
})
Expand Down
1 change: 1 addition & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ github.com/NYTimes/gziphandler v0.0.0-20170623195520-56545f4a5d46/go.mod h1:3wb0
github.com/NYTimes/gziphandler v1.1.1/go.mod h1:n/CVRwUEOgIxrgPvAQhUUr9oeUtvrhMomdKFjzJNB0c=
github.com/OneOfOne/xxhash v1.2.2/go.mod h1:HSdplMjZKSmBqAxg5vPj2TmRDmfkzw+cTzAElWljhcU=
github.com/PuerkitoBio/purell v1.1.1/go.mod h1:c11w/QuzBsJSee3cPx9rAFu61PvFxuPbtSwDGJws/X0=
github.com/PuerkitoBio/purell v1.1.1/go.mod h1:c11w/QuzBsJSee3cPx9rAFu61PvFxuPbtSwDGJws/X0=
github.com/PuerkitoBio/urlesc v0.0.0-20170810143723-de5bf2ad4578/go.mod h1:uGdkoq3SwY9Y+13GIhn11/XLaGBb4BfwItxLd5jeuXE=
github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=
github.com/alecthomas/template v0.0.0-20190718012654-fb15b899a751/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=
Expand Down