Update deployment conditions #11660
Update deployment conditions #11660
Conversation
|
[test] |
Actually I would like it for 1.4 since it makes lastTransitionTime work correctly. |
|
cc: @openshift/api-review |
|
@Kargakis probably too late for 1.4 |
| @@ -437,6 +437,8 @@ type DeploymentCondition struct { | |||
| Type DeploymentConditionType | |||
| // Status of the condition, one of True, False, Unknown. | |||
| Status kapi.ConditionStatus | |||
| // The last time this condition was updated. | |||
| LastUpdateTime unversioned.Time | |||
There was a problem hiding this comment.
I prefer the LastProbeTime from pretty much all types other than deployment since this probably didn't change (update) the condition at all.
There was a problem hiding this comment.
@deads2k LastProbeTime might be confused with probes (readiness/livenes).
There was a problem hiding this comment.
@Kargakis what is the difference between LastTransitionTime and LastUpdateTime ?
There was a problem hiding this comment.
The former is updated only when the Status of the condition transitions, the latter is updated every time the condition is updated.
There was a problem hiding this comment.
@Kargakis maybe mention that in the godoc?
There was a problem hiding this comment.
LastTransitionTime godoc already mentions when it is updated
origin/pkg/deploy/api/types.go
Line 440 in b61cf87
|
I want this in 1.4 because this is a net-new API for 1.4 and I don't want to have to go through a deprecation cycle. |
|
Oh wait, it's additive only. Yes, 1.5. |
|
The main thing here is LastTransitionTime working as LastUpdateTime in 1.4 On Mon, Oct 31, 2016 at 3:16 PM, Clayton Coleman notifications@github.com
|
| @@ -77,6 +78,10 @@ func SetDeploymentCondition(status *deployapi.DeploymentConfigStatus, condition | |||
| if currentCond != nil && currentCond.Status == condition.Status && currentCond.Reason == condition.Reason { | |||
| return | |||
| } | |||
| // Preserve lastTransitionTime if we are not switching between statuses of a condition. | |||
| if currentCond != nil && currentCond.Status == condition.Status { | |||
There was a problem hiding this comment.
We definitely need this for 1.4. I don't know that we need the rest. Can you split this out?
There was a problem hiding this comment.
i'm fine merging this for 1.4
openshift-bot
added
needs-rebase
|
Evaluated for origin test up to 41da0db |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12018/) (Base Commit: de44433) |
|
LGTM |
|
[merge] |
|
#8502 [merge] |
|
Evaluated for origin merge up to 41da0db |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12018/) (Base Commit: b61cf87) (Image: devenv-rhel7_5526) |
@mfojtik brings our conditions on par with what has merged in upstream deployment conditions. Not sure if we want it for 1.4.