Adding retry loop to update #1441

vyasgun · 2021-08-30T13:09:56Z

Description

Added retry loop from client go to service and source update commands.

Changes

Added retry loop from client go to service update command.
Added a retry updated method to ping source APIs.

Reference

Fixes #1102

knative-prow-robot

@vyasgun: 0 warnings.

In response to this:

Description

Added retry loop from client go to service update command.

Changes

Added retry loop from client go to service update command.

Reference

Fixes #1102

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.

codecov · 2021-08-30T13:14:16Z

Codecov Report

Merging #1441 (91b6848) into main (e570844) will increase coverage by 0.27%.
The diff coverage is 89.26%.

@@            Coverage Diff             @@
##             main    #1441      +/-   ##
==========================================
+ Coverage   78.70%   78.98%   +0.27%     
==========================================
  Files         162      162              
  Lines        8368     8416      +48     
==========================================
+ Hits         6586     6647      +61     
+ Misses       1098     1088      -10     
+ Partials      684      681       -3

Impacted Files	Coverage Δ
pkg/kn/commands/source/ping/ping.go	`54.16% <ø> (ø)`
pkg/kn/commands/domain/update.go	`65.71% <45.45%> (-1.86%)`	⬇️
pkg/kn/commands/subscription/update.go	`68.88% <60.00%> (+2.93%)`	⬆️
pkg/kn/commands/trigger/update.go	`77.19% <71.42%> (+4.46%)`	⬆️
pkg/kn/commands/source/ping/update.go	`84.61% <91.66%> (-0.30%)`	⬇️
pkg/eventing/v1/client.go	`87.32% <100.00%> (+4.64%)`	⬆️
pkg/eventing/v1/client_mock.go	`91.89% <100.00%> (+0.22%)`	⬆️
pkg/kn/commands/source/container/update.go	`90.47% <100.00%> (+3.51%)`	⬆️
pkg/serving/v1/client.go	`82.77% <100.00%> (-0.09%)`	⬇️
pkg/serving/v1alpha1/client.go	`86.76% <100.00%> (+4.07%)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e570844...91b6848. Read the comment docs.

vyasgun · 2021-08-31T15:13:11Z

/retest

vyasgun · 2021-08-31T15:57:18Z

/retest

rhuss

Looks good to me, thanks @vyasgun for jumping on this.

I have some proposals and questions as comments, but I think we are then still missing a bunch of update operations.

ls docs/cmd/*update* | cat -

docs/cmd/kn_domain_update.md
docs/cmd/kn_service_update.md
docs/cmd/kn_source_apiserver_update.md
docs/cmd/kn_source_binding_update.md
docs/cmd/kn_source_container_update.md
docs/cmd/kn_source_ping_update.md
docs/cmd/kn_subscription_update.md
docs/cmd/kn_trigger_update.md

Also, I would change our own update mechanism in kn service update to reuse the tool-based version.

Finally, don't forget to update the CHANGELOG ;-)

rhuss · 2021-09-01T09:14:44Z

pkg/kn/commands/source/ping/update.go

@@ -19,10 +19,14 @@ import (
 	"fmt"

 	"github.com/spf13/cobra"
+	"k8s.io/client-go/util/retry"
+	"knative.dev/client/pkg/kn/commands/service"


Why do you need this dependency ? Normally commands should stay independent and don't refer to each other to avoid cyclic dependency and allow them easily to be refactored/moved.

If there is some code that can be reused, I'd propose to move it to a shared package and use it from there in service and here.

This is because I imported the MaxUpdateRetries from service package. I will change this and create a variable in the same package.

pkg/eventing/v1/client.go

dsimansk · 2021-09-07T09:33:24Z

@vyasgun is the PR complete? Or are you still looking into some leftover update functions to add retry loop?

vyasgun · 2021-09-07T11:48:48Z

@dsimansk I pushed a few more commits. It's done now and once reviewed we can merge.

pkg/eventing/v1/client_mock.go

pkg/kn/commands/subscription/update.go

dsimansk · 2021-09-09T10:31:41Z

There's one more comment, but that's not super important. In addition there're several codecov warnings, but those can be addressed as part of improving coverage story.

IMO it looks very good, nice work!
/lgtm

@rhuss if you can make another please.

rhuss · 2021-09-09T15:48:06Z

not sure that I can make it today, but will review it early (European) morning at last.

rhuss

Thanks a lot. I have a minor suggestion to simplify the call to a function (avoiding one unnecessary inner function) and some other minor comments.

Overall looks very good and I think we are close to merge it.

pkg/eventing/v1/client.go

pkg/eventing/v1/client_mock_test.go

pkg/eventing/v1/client_test.go

rhuss · 2021-09-10T07:54:43Z

pkg/eventing/v1/client_test.go

+			newTrigger := a.(client_testing.UpdateAction).GetObject()
+			name := newTrigger.(metav1.Object).GetName()
+
+			if name == "testTrigger" && attemptCount > 0 {


very nice test for checking the retry.

pkg/messaging/v1/subscriptions_client.go

pkg/serving/v1alpha1/client.go

pkg/sources/v1beta2/ping_client.go

knative-metrics-robot · 2021-09-13T11:29:31Z

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

File	Old Coverage	New Coverage	Delta
pkg/eventing/v1/client.go	88.3%	92.6%	4.3
pkg/eventing/v1/client_mock.go	94.1%	94.3%	0.2
pkg/kn/commands/domain/update.go	81.2%	80.6%	-0.6
pkg/kn/commands/source/container/update.go	91.9%	94.1%	2.2
pkg/kn/commands/subscription/update.go	80.0%	82.1%	2.1
pkg/kn/commands/trigger/update.go	81.1%	86.4%	5.2
pkg/serving/v1/client.go	91.0%	90.9%	-0.1
pkg/serving/v1alpha1/client.go	89.4%	91.9%	2.6
pkg/serving/v1alpha1/client_mock.go	90.9%	91.3%	0.4
pkg/sources/v1/container_client.go	89.5%	92.5%	3.0
pkg/sources/v1/container_client_mock.go	90.9%	91.3%	0.4
pkg/sources/v1beta2/ping_client.go	87.1%	89.6%	2.5
pkg/sources/v1beta2/ping_client_mock.go	77.3%	78.3%	1.0

rhuss · 2021-09-13T12:53:18Z

thanks a lot !

/approve
/lgtm

knative-prow-robot · 2021-09-13T12:53:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhuss, vyasgun

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~~ [rhuss]

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

Adding retry loop to service update

e655e95

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 30, 2021

knative-prow-robot reviewed Aug 30, 2021

View reviewed changes

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 30, 2021

Added UpdatePingSourceWithRetry method

74aa56c

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 30, 2021

vyasgun added 2 commits August 31, 2021 15:44

Added UpdateTriggerWithRetry function

e9363ad

Added unit test for UpdatePingSourceWithRetry

4bd7620

rhuss requested changes Sep 1, 2021

View reviewed changes

knative-prow-robot assigned rhuss Sep 1, 2021

vyasgun added 3 commits September 1, 2021 18:08

Added unit tests for UpdateTriggerWithRetry

7ddbc9e

Refactoring changes

ce4416d

Added UpdateSubscriptionWithRetry method

5f75106

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

vyasgun added 2 commits September 7, 2021 16:33

Added retry loop to domain mapping command

fe510d8

Added retry loop to container command

26f32a1

dsimansk reviewed Sep 8, 2021

View reviewed changes

pkg/eventing/v1/client_mock.go Show resolved Hide resolved

vyasgun added 2 commits September 9, 2021 13:46

Added unit tests for mock types

eb4827c

Add Changelog entry

fd1c795

dsimansk reviewed Sep 9, 2021

View reviewed changes

pkg/kn/commands/subscription/update.go Outdated Show resolved Hide resolved

knative-prow-robot assigned dsimansk Sep 9, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2021

Updated changelog and refactored update function

794deae

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2021

rhuss reviewed Sep 10, 2021

View reviewed changes

Updated variable names

91b6848

vyasgun force-pushed the pr/retry branch from fd9d888 to 91b6848 Compare September 13, 2021 11:25

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 13, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2021

knative-prow-robot merged commit 94c8d33 into knative:main Sep 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding retry loop to update #1441

Adding retry loop to update #1441

vyasgun commented Aug 30, 2021 •

edited

Loading

knative-prow-robot left a comment

codecov bot commented Aug 30, 2021 •

edited

Loading

vyasgun commented Aug 31, 2021

vyasgun commented Aug 31, 2021

rhuss left a comment

rhuss Sep 1, 2021

vyasgun Sep 1, 2021 •

edited

Loading

rhuss Sep 3, 2021

dsimansk commented Sep 7, 2021

vyasgun commented Sep 7, 2021

dsimansk commented Sep 9, 2021

rhuss commented Sep 9, 2021

rhuss left a comment

rhuss Sep 10, 2021

knative-metrics-robot commented Sep 13, 2021

rhuss commented Sep 13, 2021

knative-prow-robot commented Sep 13, 2021

Adding retry loop to update #1441

Adding retry loop to update #1441

Conversation

vyasgun commented Aug 30, 2021 • edited Loading

Description

Changes

Reference

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

codecov bot commented Aug 30, 2021 • edited Loading

Codecov Report

vyasgun commented Aug 31, 2021

vyasgun commented Aug 31, 2021

rhuss left a comment

Choose a reason for hiding this comment

rhuss Sep 1, 2021

Choose a reason for hiding this comment

vyasgun Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

rhuss Sep 3, 2021

Choose a reason for hiding this comment

dsimansk commented Sep 7, 2021

vyasgun commented Sep 7, 2021

dsimansk commented Sep 9, 2021

rhuss commented Sep 9, 2021

rhuss left a comment

Choose a reason for hiding this comment

rhuss Sep 10, 2021

Choose a reason for hiding this comment

knative-metrics-robot commented Sep 13, 2021

rhuss commented Sep 13, 2021

knative-prow-robot commented Sep 13, 2021

vyasgun commented Aug 30, 2021 •

edited

Loading

codecov bot commented Aug 30, 2021 •

edited

Loading

vyasgun Sep 1, 2021 •

edited

Loading