Use retries from knative/pkg and add UTs to reconcilers that use retries by nlopezgi · Pull Request #6657 · knative/serving

nlopezgi · 2020-01-28T17:54:29Z

Followup to #6162

Proposed Changes

As part of add retries/fallbacks for update status google/knative-gcp#508 (which is similar to Add retries and fallback to actual API calls to status updates. #6162) I sent add retry func for reconciler pkg#1008 adding the retry.go logic to knative/pkg. This PR uses new the retry.go in knative/pkg, removes the retry.go from this repo.
This PR also adds unit tests to reconcilers that actually validate the retry functionality (similar to how they were added in add retries/fallbacks for update status google/knative-gcp#508)

knative-prow-robot

Details

In response to this:

Followup to #6162

Proposed Changes

As part of google/knative-gcp#508 (which is similar to #6162) I sent knative/pkg#1008 adding the retry.go logic to knative/pkg. This PR uses new the retry.go in knative/pkg, removes the retry.go from this repo.

This PR also adds unit tests to reconcilers that actually validate the retry functionality (similar to how they were added in google/knative-gcp#508)

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

markusthoemmes

The test method is super smart, I LOVE that! 😍

nlopezgi · 2020-01-30T14:58:50Z

thanks for the detailed feedback @markusthoemmes and @taragu. PTAL .

markusthoemmes

One last nugget :)

nlopezgi · 2020-01-30T15:24:30Z

One last nugget :)

done, thanks, thought I had removed them.

knative-test-reporter-robot · 2020-01-30T15:32:54Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-unit-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-unit-tests:

pkg/reconciler/service.[build failed]

knative-metrics-robot · 2020-01-30T15:47:57Z

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/certificate/certificate.go	87.9%	89.7%	1.7
pkg/reconciler/configuration/configuration.go	85.9%	87.2%	1.3
pkg/reconciler/ingress/ingress.go	84.1%	85.2%	1.1
pkg/reconciler/metric/metric.go	87.8%	92.7%	4.9
pkg/reconciler/serverlessservice/serverlessservice.go	94.0%	95.2%	1.2
pkg/reconciler/service/service.go	88.3%	89.5%	1.2

markusthoemmes · 2020-01-30T20:18:10Z

/retest

markusthoemmes

/lgtm
/approve

Thanks! 🎉

nlopezgi · 2020-01-30T21:52:03Z

/assign @evankanderson

markusthoemmes · 2020-01-31T07:06:52Z

/assign @mattmoor

For the Gopkg stuff.

nlopezgi · 2020-02-04T15:08:03Z

@mattmoor could you please review and let me know if anything is pending here. Thanks!

evankanderson · 2020-02-07T22:06:39Z

/approve
/lgtm

knative-prow-robot · 2020-02-07T22:06:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, markusthoemmes, nlopezgi

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:

~~OWNERS~~ [evankanderson]

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

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 28, 2020

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 28, 2020

knative-prow-robot reviewed Jan 28, 2020

View reviewed changes

knative-prow-robot added area/API API objects and controllers area/autoscale area/networking labels Jan 28, 2020

knative-prow-robot requested review from ZhiminXiang, markusthoemmes, tcnghia, vagababov and yanweiguo January 28, 2020 17:55

nlopezgi mentioned this pull request Jan 28, 2020

Add retries and fallback to actual API calls to status updates. #6162

Merged

taragu reviewed Jan 28, 2020

View reviewed changes

Comment thread pkg/reconciler/autoscaling/kpa/kpa_test.go Outdated

Nicolas Lopez added 3 commits January 29, 2020 10:16

use retries from knative/pkg and add tests for reconcilers with retries

5c2a6fd

update deps

b580995

address review comments

af36a11

nlopezgi force-pushed the retriesFromUpstream branch from 11cc7ca to e0f5f3f Compare January 29, 2020 15:19

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 29, 2020

resolve merge conflict

e4712cc

nlopezgi force-pushed the retriesFromUpstream branch 2 times, most recently from 9a76e6b to f6ec42d Compare January 29, 2020 15:48

resolve licenses and codegen issues

f6c4ea3

nlopezgi force-pushed the retriesFromUpstream branch from f6ec42d to f6c4ea3 Compare January 29, 2020 15:49

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 29, 2020

nlopezgi mentioned this pull request Jan 29, 2020

Add retries and fallback to actual API calls to status updates. knative/eventing#2465

Closed

taragu reviewed Jan 29, 2020

View reviewed changes

Comment thread pkg/reconciler/autoscaling/hpa/hpa_test.go Outdated

markusthoemmes reviewed Jan 30, 2020

View reviewed changes

Comment thread pkg/reconciler/autoscaling/hpa/hpa_test.go Outdated

review comments, partial

720c0df

also check subresource

5c5430c

markusthoemmes reviewed Jan 30, 2020

View reviewed changes

Comment thread pkg/reconciler/certificate/certificate.go

remove retry.go from this repo

70ca0fa

last one I had forgotten to update

68b9bd7

markusthoemmes approved these changes Jan 30, 2020

View reviewed changes

knative-prow-robot assigned markusthoemmes Jan 30, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2020

knative-prow-robot assigned evankanderson Jan 30, 2020

knative-prow-robot assigned mattmoor Jan 31, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2020

knative-prow-robot merged commit 1c0f9ab into knative:master Feb 7, 2020

Conversation

nlopezgi commented Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

Uh oh!

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nlopezgi commented Jan 30, 2020

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nlopezgi commented Jan 30, 2020

Uh oh!

knative-test-reporter-robot commented Jan 30, 2020

Uh oh!

knative-metrics-robot commented Jan 30, 2020

Uh oh!

markusthoemmes commented Jan 30, 2020

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

nlopezgi commented Jan 30, 2020

Uh oh!

markusthoemmes commented Jan 31, 2020

Uh oh!

nlopezgi commented Feb 4, 2020

Uh oh!

evankanderson commented Feb 7, 2020

Uh oh!

knative-prow-robot commented Feb 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

nlopezgi commented Jan 28, 2020 •

edited

Loading