Skip to content

MAO: Stop setting statusProgressing=true when resyincing same version #217

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 23 additions & 3 deletions Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions pkg/operator/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,18 +38,20 @@ func (optr *Operator) statusProgressing() error {
glog.Errorf("error getting current versions: %v", err)
return err
}

var isProgressing osconfigv1.ConditionStatus
var message string
if !reflect.DeepEqual(desiredVersions, currentVersions) {
message = fmt.Sprintf("Progressing towards %s", optr.printOperandVersions())
isProgressing = osconfigv1.ConditionTrue
} else {
message = fmt.Sprintf("Running resync for %s", optr.printOperandVersions())
isProgressing = osconfigv1.ConditionFalse
}

conds := []osconfigv1.ClusterOperatorStatusCondition{
{
Type: osconfigv1.OperatorProgressing,
Status: osconfigv1.ConditionTrue,
Status: isProgressing,
Copy link
Member

Choose a reason for hiding this comment

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

Looks good :) can you also add a test here [1] to verify the condition status is true when desiredVersions != currentVersions and false when both are equal?

[1] https://github.com/openshift/machine-api-operator/blob/master/pkg/operator/status_test.go

Copy link
Member

@enxebre enxebre Feb 21, 2019

Choose a reason for hiding this comment

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

+1, also including failing=false and progressing=false check here would be nice https://github.com/openshift/machine-api-operator/blob/master/test/e2e/operator_expectations.go#L68

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reason: string(ReasonSyncing),
Message: message,
},
Expand Down
58 changes: 56 additions & 2 deletions pkg/operator/status_test.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package operator

import (
"testing"

osconfigv1 "github.com/openshift/api/config/v1"
fakeconfigclientset "github.com/openshift/client-go/config/clientset/versioned/fake"
"github.com/stretchr/testify/assert"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"testing"
)

func TestPrintOperandVersions(t *testing.T) {
Expand All @@ -25,3 +27,55 @@ func TestPrintOperandVersions(t *testing.T) {
t.Errorf("Expected: %s, got: %s", expectedOutput, got)
}
}

func TestOperatorStatusProgressing(t *testing.T) {
type tCase struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: turn this into an anonymous struct.

oV []osconfigv1.OperandVersion
expected osconfigv1.ConditionStatus
}
tCases := []tCase{
{
oV: []osconfigv1.OperandVersion{
{
Name: "operator",
Version: "1.0",
},
},
expected: osconfigv1.ConditionFalse,
},
{
oV: []osconfigv1.OperandVersion{
{
Name: "operator",
Version: "2.0",
},
},
expected: osconfigv1.ConditionTrue,
},
}

optr := Operator{
operandVersions: []osconfigv1.OperandVersion{
{
Name: "operator",
Version: "1.0",
},
},
}
for i, tc := range tCases {
co := &osconfigv1.ClusterOperator{ObjectMeta: metav1.ObjectMeta{Name: clusterOperatorName}}
co.Status.Versions = tc.oV

optr.osClient = fakeconfigclientset.NewSimpleClientset(co)
optr.statusProgressing()
o, _ := optr.osClient.ConfigV1().ClusterOperators().Get(clusterOperatorName, metav1.GetOptions{})
var condition osconfigv1.ClusterOperatorStatusCondition
for _, coCondition := range o.Status.Conditions {
if coCondition.Type == osconfigv1.OperatorProgressing {
condition = coCondition
break
}
}
assert.Equal(t, tc.expected, condition.Status, "test-case %v expected condition %v to be %v, but got %v", i, condition.Type, tc.expected, condition.Status)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied this from machine-config-operator. It prints really nice output in case of test failure. We can remove this if we don't want to add in this dependency.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see any machine-config-operator dep :) Did you update the PR meantime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ingvagabund no, I mean the 'assert' library. They are using this library, it seems nice. But I can just do a normal test here if we don't want to pull this in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(This is what machine-config-operator is using)

Copy link
Member

@ingvagabund ingvagabund Feb 22, 2019

Choose a reason for hiding this comment

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

I see :) I am completely fine with the new dependency. We might actually start using it by default. Nice improvement.

}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading