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

dsimansk · 2021-05-31T17:31:08Z

Description

@rhuss per our conversation opening the PR with refactor of waiting loop. It was reverted before 0.23 release due to unstable tests probably cause by the changes.
#1321

Changes

Refactor waiting loop to use resourceVersion

Reference

Fixes #1287

knative-prow-robot

@dsimansk: 4 warnings.

Details

In response to this:

Description

@rhuss per our conversation opening the PR with refactor of waiting loop. It was reverted before 0.23 release due to unstable tests probably cause by the changes.
#1321

Changes

Refactor waiting loop to use resourceVersion

Reference

Fixes #1287

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-31T17:31:17Z


 // 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-31T17:31:18Z

 // 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-31T17:31:18Z

 }

+func TestWaitTimeout(t *testing.T) {
+	fakeWatchApi := NewFakeWatch([]watch.Event{})


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

knative-prow-robot · 2021-05-31T17:31:18Z

+
+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.

codecov · 2021-05-31T17:34:44Z

Codecov Report

Merging #1333 (5adb9c0) into main (a905959) will increase coverage by 0.28%.
The diff coverage is 78.72%.

@@            Coverage Diff             @@
##             main    #1333      +/-   ##
==========================================
+ Coverage   74.09%   74.37%   +0.28%     
==========================================
  Files         160      160              
  Lines        8202     8215      +13     
==========================================
+ Hits         6077     6110      +33     
+ Misses       1420     1402      -18     
+ Partials      705      703       -2

Impacted Files	Coverage Δ
pkg/wait/poll_watcher.go	`55.07% <0.00%> (-0.81%)`	⬇️
pkg/serving/v1/client.go	`62.79% <64.70%> (+5.18%)`	⬆️
pkg/eventing/v1/client.go	`81.81% <100.00%> (+2.65%)`	⬆️
pkg/serving/v1/client_mock.go	`93.42% <100.00%> (+0.27%)`	⬆️
pkg/wait/wait_for_ready.go	`73.68% <100.00%> (+7.33%)`	⬆️
pkg/wait/test_wait_helper.go	`100.00% <0.00%> (+5.00%)`	⬆️

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 a905959...5adb9c0. Read the comment docs.

dsimansk · 2021-05-31T20:01:29Z

/test all

rhuss · 2021-06-01T12:03:20Z

/retest

rhuss · 2021-06-01T12:03:51Z

/test pull-knative-client-integration-tests
/test pull-knative-client-integration-tests-latest-release

rhuss · 2021-06-01T16:52:11Z

/test pull-knative-client-integration-tests
/test pull-knative-client-integration-tests-latest-release

rhuss · 2021-06-01T17:35:58Z

/test pull-knative-client-integration-tests
/test pull-knative-client-integration-tests-latest-release

maximilien

Ping me when you are ready for a second pair of eyes review on this. Best.

rhuss · 2021-06-08T13:37:21Z

/test pull-knative-client-integration-tests
/test pull-knative-client-integration-tests-latest-release

dsimansk · 2021-06-22T12:44:57Z

/retest

navidshaikh · 2021-06-22T13:39:39Z

/assign @rhuss

dsimansk · 2021-06-24T09:26:59Z

/test all

rhuss · 2021-06-24T10:29:22Z

/retest

rhuss · 2021-06-28T06:35:15Z

@dsimansk any idea how we should proceed with this PR ? Do you remember the original error that we have observed ? Doesn't look like that it occurs very often ...

rhuss · 2021-06-29T07:11:46Z

We decided in the WG call that we want to merge this or after the release, and then have 6 week time to detect and fix any potential issues that only occurred once in the past (not sure anymore whether this was a flake)

dsimansk · 2021-07-01T10:54:25Z

/test all

knative-metrics-robot · 2021-07-01T13:36:19Z

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%	87.7%	1.9
pkg/serving/v1/client.go	69.3%	75.0%	5.7
pkg/serving/v1/client_mock.go	94.2%	94.4%	0.2
pkg/wait/wait_for_ready.go	76.1%	81.0%	4.9

rhuss

Thanks !

Let's get the merged back in and watch the nightly builds

/lgtm
/approve

knative-prow-robot · 2021-07-01T19:04:58Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

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 31, 2021

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

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

knative-prow-robot reviewed May 31, 2021

View reviewed changes

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

maximilien reviewed Jun 2, 2021

View reviewed changes

knative-prow-robot assigned rhuss Jun 22, 2021

dsimansk mentioned this pull request Jun 24, 2021

Race in waiting loop #1349

Closed

dsimansk force-pushed the waiting-loop-refactor branch from acc4baa to 8c57394 Compare June 25, 2021 12:58

dsimansk added 2 commits July 1, 2021 12:29

Refactor waiting loop to use resourceVersion

78ff64c

Add test coverage for delete ops

375fc85

dsimansk force-pushed the waiting-loop-refactor branch from 8c57394 to 375fc85 Compare July 1, 2021 10:29

dsimansk changed the title ~~WIP: Refactor waiting loop to use resourceVersion~~ Refactor waiting loop to use resourceVersion Jul 1, 2021

Reformat code

4fb542d

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 Jul 1, 2021

Move helper function to test file

7aa1aee

dsimansk force-pushed the waiting-loop-refactor branch from 7f3804f to 7aa1aee Compare July 1, 2021 11:38

Add more test coverage

5adb9c0

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 Jul 1, 2021

rhuss reviewed Jul 1, 2021

View reviewed changes

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

knative-prow-robot merged commit 090ba0c into knative:main Jul 1, 2021

Conversation

dsimansk commented May 31, 2021

Description

Changes

Reference

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

Uh oh!

knative-prow-robot May 31, 2021

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot May 31, 2021

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot May 31, 2021

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot May 31, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

dsimansk commented May 31, 2021

Uh oh!

rhuss commented Jun 1, 2021

Uh oh!

rhuss commented Jun 1, 2021

Uh oh!

rhuss commented Jun 1, 2021

Uh oh!

rhuss commented Jun 1, 2021

Uh oh!

maximilien left a comment

Choose a reason for hiding this comment

Uh oh!

rhuss commented Jun 8, 2021

Uh oh!

dsimansk commented Jun 22, 2021

Uh oh!

navidshaikh commented Jun 22, 2021

Uh oh!

dsimansk commented Jun 24, 2021

Uh oh!

rhuss commented Jun 24, 2021

Uh oh!

rhuss commented Jun 28, 2021

Uh oh!

rhuss commented Jun 29, 2021

Uh oh!

dsimansk commented Jul 1, 2021

Uh oh!

knative-metrics-robot commented Jul 1, 2021

Uh oh!

rhuss left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Jul 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented May 31, 2021 •

edited

Loading