Skip non modified event first while waiting for ready #1390

cardil · 2021-07-19T18:15:15Z

Description

Allow using wait for ready with dynamic kube client, as before this change, an error was received on Added event with unstructured object, which by default doesn't have .status field.

Changes

🐛 Bug fix of WaitForReady should check for event type before trying to read generation number #1389 - Skipping on non-modified event while waiting for ready

Reference

Fixes #1389

knative-prow-robot

@cardil: 0 warnings.

In response to this:

Description

Allow using wait for ready with dynamic kube client, as before this change, an error was received on Added event with unstructured object, which by default doesn't have .status field.

Changes

🐛 Bug fix of WaitForReady should check for event type before trying to read generation number #1389 - Skipping on non-modified event while waiting for ready

Reference

Fixes #1389

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-07-19T18:19:24Z

Codecov Report

Merging #1390 (6479023) into main (e26d5f2) will increase coverage by 1.68%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1390      +/-   ##
==========================================
+ Coverage   76.51%   78.20%   +1.68%     
==========================================
  Files         160      160              
  Lines        8234     8234              
==========================================
+ Hits         6300     6439     +139     
+ Misses       1229     1104     -125     
+ Partials      705      691      -14

Impacted Files	Coverage Δ
pkg/wait/test_wait_helper.go	`100.00% <100.00%> (ø)`
pkg/wait/wait_for_ready.go	`87.71% <100.00%> (+5.26%)`	⬆️
pkg/serving/config_changes.go	`84.74% <0.00%> (+6.77%)`	⬆️
pkg/serving/v1/gitops.go	`85.22% <0.00%> (+11.36%)`	⬆️
pkg/serving/v1/apply.go	`80.32% <0.00%> (+11.47%)`	⬆️
pkg/kn/commands/namespaced.go	`81.08% <0.00%> (+16.21%)`	⬆️
pkg/serving/v1/client.go	`81.86% <0.00%> (+19.06%)`	⬆️
pkg/kn/commands/describe.go	`89.69% <0.00%> (+20.61%)`	⬆️
pkg/kn/commands/types.go	`80.23% <0.00%> (+26.74%)`	⬆️
... and 1 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 e26d5f2...6479023. Read the comment docs.

maximilien

/ok-to-test

pkg/wait/wait_for_ready_test.go

itsmurugappan · 2021-07-20T02:07:39Z

lgtm, but to pass the code cov test, the lines that were changed need to have more than 76.51 % coverage.

dsimansk · 2021-07-20T17:08:15Z

lgtm, but to pass the code cov test, the lines that were changed need to have more than 76.51 % coverage.

@cardil can you please take a look at the coverage? We've discussed on WG call that if it's too cumbersome to reach some of the statements it can be even merged without reaching the required number, just let us know please. :)

dsimansk · 2021-07-21T15:05:49Z

pkg/wait/wait_for_ready_test.go

+func peUnstructured(tb testing.TB) func(name string) ([]watch.Event, int) {
+	return func(name string) ([]watch.Event, int) {


knative-metrics-robot · 2021-07-21T16:27:51Z

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/wait/wait_for_ready.go	90.0%	93.0%	3.0

cardil · 2021-07-21T17:02:28Z

@itsmurugappan @dsimansk I did raise the coverage on reported method, although it's a synthetic example that shouldn't occur with duck elements, which always should have generation and observed generation. Now, this PR passes all the gates.

cardil · 2021-07-21T17:03:19Z

/cc @itsmurugappan

itsmurugappan

/lgtm

itsmurugappan · 2021-07-21T17:06:11Z

cool, coverage also jumping by 1.68% 👍

dsimansk · 2021-07-21T18:48:35Z

/lgtm

dsimansk · 2021-07-21T18:49:29Z

Since it's addressing all the review comments, imo it's good to go.
/approve

knative-prow-robot · 2021-07-21T18:49:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cardil, dsimansk, itsmurugappan

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

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

* Fixing the addressable resolver * Properly working address resolver * Working fake & it tests (apart those affected by knative/client#1389) * Upgrade to latest knative * Use interim fork until merge of knative/client#1390 * Allow addressable URI to be empty, and by default * Remove interim personal fork

cardil added 2 commits July 19, 2021 20:04

Reproduction of knative#1389

7ca0769

Fixing knative#1389

bd369c9

knative-prow-robot reviewed Jul 19, 2021

View reviewed changes

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

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

cardil added a commit to cardil/kn-plugin-event that referenced this pull request Jul 19, 2021

Use interim fork until merge of knative/client#1390

9a34882

cardil mentioned this pull request Jul 19, 2021

Fixing the addressable resolver knative-extensions/kn-plugin-event#38

Merged

cardil added a commit to cardil/kn-plugin-event that referenced this pull request Jul 19, 2021

Use interim fork until merge of knative/client#1390

c951d58

maximilien reviewed Jul 19, 2021

View reviewed changes

pkg/wait/wait_for_ready_test.go Outdated Show resolved Hide resolved

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

Removal of panic in test

6154fd4

dsimansk reviewed Jul 21, 2021

View reviewed changes

Cover the "impossible" missing generation case

6479023

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

cardil requested a review from dsimansk July 21, 2021 17:02

knative-prow-robot requested a review from itsmurugappan July 21, 2021 17:03

itsmurugappan approved these changes Jul 21, 2021

View reviewed changes

knative-prow-robot assigned itsmurugappan Jul 21, 2021

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

cardil requested a review from maximilien July 21, 2021 17:10

knative-prow-robot assigned dsimansk Jul 21, 2021

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

knative-prow-robot merged commit a7df7c7 into knative:main Jul 21, 2021

cardil deleted the bugfix/1389-skip-non-modified-first branch July 21, 2021 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip non modified event first while waiting for ready #1390

Skip non modified event first while waiting for ready #1390

cardil commented Jul 19, 2021

knative-prow-robot left a comment

codecov bot commented Jul 19, 2021 •

edited

Loading

maximilien left a comment

itsmurugappan commented Jul 20, 2021

dsimansk commented Jul 20, 2021

dsimansk Jul 21, 2021

knative-metrics-robot commented Jul 21, 2021

cardil commented Jul 21, 2021

cardil commented Jul 21, 2021

itsmurugappan left a comment

itsmurugappan commented Jul 21, 2021

dsimansk commented Jul 21, 2021

dsimansk commented Jul 21, 2021

knative-prow-robot commented Jul 21, 2021

		func peUnstructured(tb testing.TB) func(name string) ([]watch.Event, int) {
		return func(name string) ([]watch.Event, int) {

Skip non modified event first while waiting for ready #1390

Skip non modified event first while waiting for ready #1390

Conversation

cardil commented Jul 19, 2021

Description

Changes

Reference

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

codecov bot commented Jul 19, 2021 • edited Loading

Codecov Report

maximilien left a comment

Choose a reason for hiding this comment

itsmurugappan commented Jul 20, 2021

dsimansk commented Jul 20, 2021

dsimansk Jul 21, 2021

Choose a reason for hiding this comment

knative-metrics-robot commented Jul 21, 2021

cardil commented Jul 21, 2021

cardil commented Jul 21, 2021

itsmurugappan left a comment

Choose a reason for hiding this comment

itsmurugappan commented Jul 21, 2021

dsimansk commented Jul 21, 2021

dsimansk commented Jul 21, 2021

knative-prow-robot commented Jul 21, 2021

codecov bot commented Jul 19, 2021 •

edited

Loading