-
Notifications
You must be signed in to change notification settings - Fork 306
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
fix: Do not set job timeout extra property if None #1987
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
job_timeout_ms
to None d4a5eaf
to
f76a4fe
Compare
f76a4fe
to
431802f
Compare
@Linchin updated, could you review? |
f2e77dc
to
b30ca9f
Compare
07ca2d2
to
602c0b5
Compare
786a5ef
to
6417497
Compare
@Linchin rebased on the latest main branch |
Thank you for making the contribution and fix this bug for us! I wonder if you would like to lint the code, as this seems to be the only test that's failing. I haven't had the time to do it. To lint you need to install nox ( |
e73a9ae
to
166ca14
Compare
Fixed linting, you can re-run checks now |
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.
LGTM
Description
job_timeout_ms
declared as optional (see docs), but setting it as None results in error:because of allowing None, but not handling it properly. This fix doesn't cast None to a string in setter. Getter still works as expected, but we do not send invalid value in remote call.
Simple script to reproduce