Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added flag to configure wait-window between intermediate errors durin… #1645

Merged
merged 3 commits into from
Apr 13, 2022
Merged

Added flag to configure wait-window between intermediate errors durin… #1645

merged 3 commits into from
Apr 13, 2022

Conversation

vyasgun
Copy link
Contributor

@vyasgun vyasgun commented Apr 11, 2022

…g service create

Description

Added the flag --wait-window for configuring the default error window between intermediate false Ready conditions when a service is coming up

Changes

  • Added the flag
  • Changed existing signature for Wait function to accommodate the flag value

Reference

Fixes #1023

knative-prow[bot]
knative-prow bot reviewed Apr 11, 2022
View reviewed changes
Copy link

@knative-prow knative-prow bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vyasgun: 3 warnings.

In response to this:

…g service create

Description

Added the flag --wait-window for configuring the default error window between intermediate false Ready conditions when a service is coming up

Changes

  • Added the flag
  • Changed existing signature for Wait function to accommodate the flag value

Reference

Fixes #1023

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.

pkg/serving/v1/client.go Outdated Show resolved Hide resolved
pkg/serving/v1/client_mock.go Outdated Show resolved Hide resolved
pkg/serving/v1/gitops.go Outdated Show resolved Hide resolved
@knative-prow knative-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 11, 2022
dsimansk
dsimansk reviewed Apr 11, 2022
View reviewed changes
pkg/serving/v1/client.go Outdated
@@ -89,7 +89,7 @@ type KnServingClient interface {

// Wait for a service to become ready, but not longer than provided timeout.
// Return error and how long has been waited
WaitForService(ctx context.Context, name string, timeout time.Duration, msgCallback wait.MessageCallback) (error, time.Duration)
WaitForService(ctx context.Context, name string, timeout, errorWindow time.Duration, msgCallback wait.MessageCallback) (error, time.Duration)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Note for the future :))
I wonder if it was worth to introduce simple waitConfig structure to represent those timeout values. The number of arguments is still manageable, until it won't grow too much. :) Maybe an overall refactor of wait would help, but that's certainly outside of the PR's scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I added the waitConfig struct for now

dsimansk
dsimansk requested changes Apr 11, 2022
View reviewed changes
Copy link
Contributor

@dsimansk dsimansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There're a few more functions to be updated to reflect the changes in unit tests particularly.

@vyasgun vyasgun force-pushed the pr/error-window branch 2 times, most recently from 611aa68 to fbf9769 Compare April 12, 2022 20:49
@codecov
Copy link

codecov bot commented Apr 12, 2022 •
edited
Loading

Codecov Report

Merging #1645 (18765de) into main (0ac405d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1645      +/-   ##
==========================================
+ Coverage   79.28%   79.30%   +0.01%     
==========================================
  Files         171      171              
  Lines       12914    12925      +11     
==========================================
+ Hits        10239    10250      +11     
  Misses       1955     1955              
  Partials      720      720
Impacted Files Coverage Δ
pkg/kn/commands/service/create.go 81.12% <100.00%> (+0.39%) ⬆️
pkg/kn/commands/service/service.go 78.04% <100.00%> (ø)
pkg/kn/commands/service/update.go 80.85% <100.00%> (+0.85%) ⬆️
pkg/kn/commands/wait_flags.go 100.00% <100.00%> (ø)
pkg/serving/v1/client.go 81.93% <100.00%> (ø)
pkg/serving/v1/client_mock.go 92.96% <100.00%> (ø)
pkg/serving/v1/gitops.go 84.29% <100.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 0ac405d...18765de. Read the comment docs.

@vyasgun
Copy link
Contributor Author

vyasgun commented Apr 13, 2022

/retest

dsimansk
dsimansk reviewed Apr 13, 2022
View reviewed changes
pkg/serving/v1/client.go
Comment on lines +51 to +55
type WaitConfig struct {
Timeout time.Duration
ErrorWindow time.Duration
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 💯

dsimansk
dsimansk approved these changes Apr 13, 2022
View reviewed changes
Copy link
Contributor

@dsimansk dsimansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

/approve
/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2022
@knative-prow
Copy link

knative-prow bot commented Apr 13, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, 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:

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

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 13, 2022
@knative-prow knative-prow bot merged commit 2929f38 into knative:main Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knative-prow knative-prow[bot] knative-prow[bot] left review comments

@dsimansk dsimansk dsimansk approved these changes

Assignees

@dsimansk dsimansk

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make error-window when waiting for a service to be ready configurable
2 participants
@vyasgun @dsimansk