pkg/controller/status/status: Set reasons for conditions #114

wking · 2020-05-21T03:40:18Z

We've been light on reasons for expected conditions like Progressing=False since 80ab923. But if we feel like we have a message we want to set to help humans understand the condition, we should be setting a reason string for machines too. AsExpected follows the library-go precedent.

The Degraded reason I'm adding here isn't a great fit for a progressing reason, but does match the current logic used to set Progressing=True. The insights operator doesn't actually supply any in-cluster APIs beyond ClusterOperator as far as I can tell, so it's not clear to me what sort of progressing it would do except its internal intialization (which I give a new Initializing reason). But adding an odd reason seemed easy enough, and we can always circle back later and remove this Progressing=True case if we decide we don't need it.

We've been light on reasons for expected conditions like Progressing=False since 80ab923 (Initial support operator commit, 2019-04-02). But if we feel like we have a message we want to set to help humans understand the condition, we should be setting a reason string for machines too. AsExpected follows the library-go precedent [1]. The "Degraded" reason I'm adding here isn't a great fit for a progressing reason, but does match the current logic used to set Progressing=True. The insights operator doesn't actually supply any in-cluster APIs beyond ClusterOperator as far as I can tell, so it's not clear to me what sort of progressing it would do except its internal intialization (which I give a new "Initializing" reason). But adding an odd reason seemed easy enough, and we can always circle back later and remove this Progressing=True case if we decide we don't need it. [1]: https://github.com/openshift/library-go/blob/94c59dec54be25c8527e51e8c0a885712aeb01b5/pkg/operator/status/condition.go#L67

iNecas · 2020-05-21T10:04:55Z

/retest

martinkunc · 2020-05-28T13:07:32Z

@wking, could you please add BZ Bug prefix, which would allow us to merge it historically. Is it ok to merge all the way to 4.2 ?

wking · 2020-05-28T18:35:24Z

...could you please add BZ Bug prefix...

Is this serious enough to backport? 4.2 at least is maintenance mode, and I'm pretty sure this does not meet that bar. I was expecting it to land next week once 4.5 forks off master and never be backported at all.

martinkunc · 2020-05-29T09:55:48Z

Ok, I agree this is a minor change. The reasoning behind this was to allow future collaborators effortless automatic merge as much as possible.

martinkunc · 2020-05-29T09:55:55Z

/lgtm

openshift-ci-robot · 2020-05-29T09:56:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: martinkunc, wking

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:

~~OWNERS~~ [martinkunc]

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

martinkunc · 2020-05-29T16:49:50Z

@wking Actually, I could backport this for you if you dont mind. I am sure being proactive will save future mergers some, even minimal effort. There are many people committing to repo nowadays.

openshift-bot · 2020-06-01T13:23:52Z

/retest