Refactor waiting loop to use resourceVersion by dsimansk · Pull Request #1301 · knative/client

dsimansk · 2021-05-04T20:48:13Z

Description

Changes

Use resourceVersion in *Delete functions to get replay of events and prevent
Re-enable retry logic when watch channel is closed before timeout

Reference

Fixes #1287

/cc @rhuss

knative-prow-robot

@dsimansk: 4 warnings.

Details

In response to this:

Description

Changes

Use resourceVersion in *Delete functions to get replay of events and prevent

Re-enable retry logic when watch channel is closed before timeout

Reference

Fixes #1287

/cc @rhuss

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.

knative-prow-robot · 2021-05-04T20:48:23Z

 	return polling, nil
 }

+func NewWatcherWithVersion(ctx context.Context, watchFunc watchF, c rest.Interface, ns string, resource string, name string, initialResourceVersion string, timeout time.Duration) (watch.Interface, error) {


Golint comments: exported function NewWatcherWithVersion should have comment or be unexported. More info.

knative-prow-robot · 2021-05-04T20:48:23Z

 // msgCallback gets called for every event with an 'Ready' condition == UNKNOWN with the event's message.
-func (w *waitForReadyConfig) Wait(ctx context.Context, watcher watch.Interface, name string, options Options, msgCallback MessageCallback) (error, time.Duration) {
-
+func (w *waitForReadyConfig) Wait(ctx context.Context, name string, initialVersion string, options Options, msgCallback MessageCallback) (error, time.Duration) {


Golint arg-order: error should be the last type when returning multiple items.

knative-prow-robot · 2021-05-04T20:48:23Z


 // Wait until the expected EventDone is satisfied
-func (w *waitForEvent) Wait(ctx context.Context, watcher watch.Interface, name string, options Options, msgCallback MessageCallback) (error, time.Duration) {
+func (w *waitForEvent) Wait(ctx context.Context, name string, initialVersion string, options Options, msgCallback MessageCallback) (error, time.Duration) {


Golint arg-order: error should be the last type when returning multiple items.

knative-prow-robot · 2021-05-04T20:48:23Z

+
+func TestAddWaitForReadyWithChannelClose(t *testing.T) {
+	for i, tc := range prepareTestCases("test-service") {
+		fakeWatchApi := NewFakeWatch(tc.events)


Golint naming: var fakeWatchApi should be fakeWatchAPI. More info.

maximilien

/ok-to-test
Left some feedback, especially related to some missing coverage. Of course we cannot cover all but at least see if you can solve one --- some are same in different locations.

maximilien · 2021-05-05T20:44:29Z

+	if err != nil {
+		return false, false, err
+	}
+	defer watcher.Stop()


I can comment lines 150-152 and all tests passing. Similar on the other changes. My guess is that your tests are not covering the cases for failed timeout or similar?

maximilien · 2021-05-05T20:47:19Z

+		c, ns, resource, name, timeout, make(chan bool), make(chan watch.Event), &sync.WaitGroup{},
+		newTickerPollInterval(time.Second), nativePoll(ctx, c, ns, resource, name)}
+	polling.start()
+	return polling, nil


I can comment this line and all tests pass. So missing coverage.

maximilien · 2021-05-05T20:49:59Z

+	if err != nil {
+		return err
+	}


I can comment lines 253-255 and all tests passing

maximilien · 2021-05-05T20:51:26Z

+	if err != nil {
+		return err
+	}


Same as previous...

dsimansk · 2021-05-06T14:28:33Z

Left some feedback, especially related to some missing coverage. Of course we cannot cover all but at least see if you can solve one --- some are same in different locations.

@maximilien thanks! Sure, I'll take a pass on the test. The waiting loop is pretty tricky anyway, so more coverage is definitely good goal.

rhuss · 2021-05-07T10:04:31Z

@dsimansk Thanks a lot ! I won't be able to make review this week, but will have a look asap early next week.

maximilien · 2021-05-07T15:40:10Z

Sounds good. Thanks @dsimansk

knative-metrics-robot · 2021-05-13T12:15:11Z

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	85.8%	86.0%	0.1
pkg/serving/v1/client.go	69.3%	69.0%	-0.3
pkg/wait/wait_for_ready.go	76.1%	81.0%	4.9

navidshaikh

/lgtm

knative-prow-robot · 2021-05-17T16:28:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: navidshaikh

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

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

This reverts commit 9f6ec31.

* Revert "Refactor waiting loop to use resourceVersion (#1301)" This reverts commit 9f6ec31. * Remove changelog entry

WIP: Refactor waiting loop to use resourceVersion

e825161

knative-prow-robot requested a review from rhuss May 4, 2021 20:48

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 4, 2021

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

knative-prow-robot reviewed May 4, 2021

View reviewed changes

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 4, 2021

dsimansk added 2 commits May 4, 2021 23:54

Remove testing println

bd81601

Fix error return in DeleteService

9635aeb

maximilien suggested changes May 5, 2021

View reviewed changes

knative-prow-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 5, 2021

Add more tests to reflect review feedback

83e9738

dsimansk changed the title ~~WIP: Refactor waiting loop to use resourceVersion~~ Refactor waiting loop to use resourceVersion May 13, 2021

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 13, 2021

navidshaikh approved these changes May 17, 2021

View reviewed changes

knative-prow-robot assigned navidshaikh May 17, 2021

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

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

knative-prow-robot merged commit 9f6ec31 into knative:main May 17, 2021

dsimansk added a commit to dsimansk/client that referenced this pull request May 18, 2021

Revert "Refactor waiting loop to use resourceVersion (knative#1301)"

b6d0617

This reverts commit 9f6ec31.

knative-prow-robot pushed a commit that referenced this pull request May 18, 2021

Revert "Refactor waiting loop to use resourceVersion (#1321)

fc4bc4f

* Revert "Refactor waiting loop to use resourceVersion (#1301)" This reverts commit 9f6ec31. * Remove changelog entry

Conversation

dsimansk commented May 4, 2021

Description

Changes

Reference

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximilien left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsimansk commented May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhuss commented May 7, 2021

Uh oh!

maximilien commented May 7, 2021

Uh oh!

knative-metrics-robot commented May 13, 2021

Uh oh!

navidshaikh left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented May 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dsimansk commented May 6, 2021 •

edited

Loading