-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
validate setupTimeout option #3898
validate setupTimeout option #3898
Conversation
👋 @tsukasaI this PR is now empty. I guessed you tried to revert them before making the changes discussed in the issue, but those have not been pushed. Just pointing this out in case you think that was done and are waiting on us to review this - there is currently no code to review. |
Oh, sorry. |
lib/options.go
Outdated
if opts.SetupTimeout.Valid && opts.SetupTimeout.Duration <= 0 { | ||
return o, errors.New("setupTimeout must be positive") | ||
} else if opts.SetupTimeout.Valid { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't think changing Apply signature is warranted.
We do validation checks for most of the options, and they all happen in the respected places instead of in Apply - so please do not change the signature of Apply - that will also make the change a lot smaller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mstoykov
Thank you for comment.
I updated to move in Validate method.
Is it reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tsukasaI LGTM! Thank you for your contribution!
What?
Validate setuptimeout option
Why?
Checklist
make lint
) and all checks pass.make tests
) and all tests pass.Related PR(s)/Issue(s)
#1174
Manual test
Make t.js.
Run this command with setupTimeone is 0s, and obtain error.