Skip to content
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

This adds LastTransitionTime to all knative/serving resources. #1142

Merged
merged 1 commit into from
Jun 11, 2018

Conversation

mattmoor
Copy link
Member

This is automatically populated by setCondition.

Other misc changes:

  • Switch route_test.go to us compact array syntax.
  • Switch route_test.go to the same pattern for checking conditions as used by the other controller tests.

Fixes: #1141

@google-prow-robot google-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 11, 2018
@mattmoor
Copy link
Member Author

WIP until baseline changes go through.

@knative-metrics-robot
Copy link

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File Old Coverage New Coverage Delta
pkg/apis/serving/v1alpha1/route_types.go 85.0% 85.4% 0.4
pkg/apis/serving/v1alpha1/service_types.go 95.8% 96.6% 0.7
pkg/apis/serving/v1alpha1/service_types_test.go 95.8% 96.6% 0.7
pkg/apis/serving/v1alpha1/configuration_types.go 96.0% 97.5% 1.5
pkg/apis/serving/v1alpha1/configuration_types_test.go 96.0% 97.5% 1.5
pkg/controller/configuration/configuration.go 76.9% 73.9% -3.0
pkg/controller/configuration/configuration_test.go 76.9% 73.9% -3.0

*TestCoverage feature is being tested, do not rely on any info here yet

This is automatically populated by `setCondition`.

Other misc changes:
 * Switch `route_test.go` to us compact array syntax.
 * Switch `route_test.go` to the same pattern for checking conditions as used by the other controller tests.

Fixes: knative/serving#1141
@mattmoor mattmoor force-pushed the last-transition-time branch from 2dd5350 to 0c1a2b1 Compare June 11, 2018 22:10
@mattmoor mattmoor changed the title [WIP]This adds LastTransitionTime to all knative/serving resources. This adds LastTransitionTime to all knative/serving resources. Jun 11, 2018
@google-prow-robot google-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 11, 2018
@knative-metrics-robot
Copy link

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File Old Coverage New Coverage Delta
pkg/apis/serving/v1alpha1/service_types.go 96.4% 96.6% 0.1
pkg/apis/serving/v1alpha1/configuration_types.go 97.4% 97.5% 0.1
pkg/apis/serving/v1alpha1/route_types.go 85.0% 85.4% 0.4

*TestCoverage feature is being tested, do not rely on any info here yet

@google-prow-robot google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2018
Copy link
Contributor

@tcnghia tcnghia left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@google-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, tcnghia

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:

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

@google-prow-robot google-prow-robot merged commit 74ac809 into knative:master Jun 11, 2018
@@ -94,6 +95,9 @@ type RouteCondition struct {

Status corev1.ConditionStatus `json:"status" description:"status of the condition, one of True, False, Unknown"`

// +optional
LastTransitionTime metav1.Time `json:"lastTransitionTime,omitempty" description:"last time the condition transit from one status to another"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: "transited"? "transitioned"?

@@ -81,6 +82,9 @@ type ServiceCondition struct {

Status corev1.ConditionStatus `json:"status" description:"status of the condition, one of True, False, Unknown"`

// +optional
LastTransitionTime metav1.Time `json:"lastTransitionTime,omitempty" description:"last time the condition transit from one status to another"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Same transit nit

google-prow-robot pushed a commit that referenced this pull request Jun 15, 2018
This field was added in #1142 but missed the codegen step.
skonto pushed a commit to skonto/serving that referenced this pull request Jul 25, 2022
…native#1142)

* Require 32 consecutive OK responses before having the endpoint ready

* The point is that we do not add the RouteInconsistencyRetryChecker to
the general Poll function but only to Check function which is used in
CheckEndpointState. So tests that directly call client.Do should not be
sending many requests.

* Separate simple and robust retry checker

* Use the multi/robust checker in CheckEndpointState
* Use the simple checker in simple client.Do/Poll calls
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants