Skip to content

Stop reporting unavailable when progressing and add support for multiple operands status#209

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
enxebre:cluster-operator-status
Feb 15, 2019
Merged

Stop reporting unavailable when progressing and add support for multiple operands status#209
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
enxebre:cluster-operator-status

Conversation

@enxebre
Copy link
Copy Markdown
Member

@enxebre enxebre commented Feb 14, 2019 •
edited
Loading

Support for setting multiple operands in version dynamically
Stop setting available=false when progressing
Compare available (status.Versions) vs desired (optr.Versions)
Aligns with openshift/machine-config-operator#351

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 14, 2019
@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Feb 14, 2019

cc @bison @derekwaynecarr

bison
bison approved these changes Feb 14, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@bison bison left a comment

Choose a reason for hiding this comment

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

/lgtm

I think this looks reasonable. At some point we should probably add additional related objects, and maybe sync those periodically. This is a good start though.

@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Feb 14, 2019

/test integration

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2019
@enxebre enxebre added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 14, 2019
@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Feb 14, 2019

/test integration

1 similar comment
@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Feb 14, 2019

/test integration

@enxebre enxebre force-pushed the cluster-operator-status branch from 305ee69 to a7d319e Compare February 14, 2019 14:08
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2019
@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Feb 14, 2019

/test integration

@enxebre enxebre force-pushed the cluster-operator-status branch from a7d319e to 2031c07 Compare February 14, 2019 14:28
@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Feb 14, 2019

/test integration

@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Feb 14, 2019

integration was failing due to minikube version incompatibilities with updateStatus/CRDs. Bumped to https://github.com/kubernetes/minikube/blob/master/CHANGELOG.md#version-0320---12212018

@bison
Copy link
Copy Markdown
Contributor

bison commented Feb 14, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2019
@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Feb 14, 2019

/test e2e-aws-operator

3 similar comments
@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Feb 14, 2019

/test e2e-aws-operator

@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Feb 14, 2019

/test e2e-aws-operator

@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Feb 14, 2019

/test e2e-aws-operator

@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Feb 15, 2019

/test e2e-aws

@enxebre enxebre removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 15, 2019
@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Feb 15, 2019 •
edited
Loading 

kubectl describe clusteroperator machine-api
Name:         machine-api
Namespace:
Labels:       <none>
Annotations:  <none>
API Version:  config.openshift.io/v1
Kind:         ClusterOperator
Metadata:
  Creation Timestamp:  2019-02-14T10:48:27Z
  Generation:          1
  Resource Version:    50347
  Self Link:           /apis/config.openshift.io/v1/clusteroperators/machine-api
  UID:                 0f5fc8a9-3046-11e9-82e1-0249059d5e68
Spec:
Status:
  Conditions:
    Last Transition Time:  <nil>
    Status:                False
    Type:                  Progressing
    Last Transition Time:  <nil>
    Message:               Cluster Machine API Operator is available at operator: v0.2.0-26-g305ee694
    Status:                True
    Type:                  Available
    Last Transition Time:  2019-02-14T11:33:34Z
    Status:                False
    Type:                  Failing
  Extension:               <nil>
  Related Objects:
    Group:
    Name:      openshift-machine-api
    Resource:  namespaces
  Versions:
    Name:     operator
    Version:  v0.2.0-26-g305ee694
Events:       <none>

frobware
frobware approved these changes Feb 15, 2019
View reviewed changes
Comment thread pkg/operator/status.go
"fmt"
"reflect"
"strings"

Copy link
Copy Markdown
Contributor

@frobware frobware Feb 15, 2019

Choose a reason for hiding this comment

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

ntt: drop blank line(s).

Copy link
Copy Markdown
Contributor

@bison bison Feb 15, 2019

Choose a reason for hiding this comment

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

The blank line between standard library imports and others is pretty standard if that's what this is referring to -- goimports does this automatically.

Copy link
Copy Markdown
Contributor

@frobware frobware Feb 15, 2019

Choose a reason for hiding this comment

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

Too late now... but I think there were multiple which is why I got picky!

@frobware
Copy link
Copy Markdown
Contributor

frobware commented Feb 15, 2019

/lgtm

@ingvagabund
Copy link
Copy Markdown
Member

ingvagabund commented Feb 15, 2019

/approve

@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented Feb 15, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 15, 2019
@paulfantom
Copy link
Copy Markdown
Contributor

paulfantom commented Feb 15, 2019

/lgtm

@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Feb 15, 2019

/test e2e-aws

@paulfantom
Copy link
Copy Markdown
Contributor

paulfantom commented Feb 15, 2019

/retest

This was referenced Feb 15, 2019
Merged
Merged
@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Feb 15, 2019

/test e2e-aws

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Feb 15, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit daa72d4 into openshift:master Feb 15, 2019
@derekwaynecarr
Copy link
Copy Markdown
Member

derekwaynecarr commented Feb 20, 2019

just saw this, heads up that i am working on some disruptive testing in this area after discovering not all components were syncing status regularly enough.

see: openshift/origin#22092

ingvagabund pushed a commit to ingvagabund/machine-api-operator that referenced this pull request Jul 11, 2019
@openshift-merge-robot
Merge pull request openshift#209 from enxebre/namespace
0e9a428 
Reduce machine controller watched scope to namespace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@frobware frobware frobware approved these changes

@vikaschoudhary16 vikaschoudhary16 Awaiting requested review from vikaschoudhary16

+1 more reviewer

@bison bison bison approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bison bison

@frobware frobware

@paulfantom paulfantom

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@enxebre @bison @frobware @ingvagabund @openshift-ci-robot @paulfantom @openshift-bot @derekwaynecarr @openshift-merge-robot