-
Notifications
You must be signed in to change notification settings - Fork 710
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 helm test #356
Fix helm test #356
Conversation
Hi @jose5918. Thanks for your PR. I'm waiting for a kubernetes or tensorflow member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. 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. |
@jose5918 Hey Jose, Can you please update the spec here too |
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.
Could you update the PR description to provide more info about what the problem was and the fix?
py/deploy.py
Outdated
@@ -114,7 +114,7 @@ def test(args): | |||
start = time.time() | |||
util.run(["helm", "test", "tf-job"]) | |||
except subprocess.CalledProcessError as e: | |||
t.failure = "helm test failed;\n" + e.output | |||
t.failure = "helm test failed;\n" + (e.output or '') |
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.
use " not ' to be consistent with the rest of the file.
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.
@jlewi Sorry I don't follow. I don't see not
in the rest of the file
Do you mean like an if else statement?
if not e:
t.failure = "helm test failed;\n"
else:
t.failure = "helm test failed;\n" + e.output
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 was referring to the quotation marks. We are using double quotes not single quotes in this file so we should be consistent.
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.
Ah got it
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.
@jlewi Ok this should be addressed
/ok-to-test |
I confirmed the test is passing in Airflow. |
Thanks for the fix! |
Helm test was failing because validation for a tfjob required that replicaSpecs for a Parameter server specify a template. Helm test failure also was not reported.
Changes made:
None
before concatenating the error.Fixes #351 and #355
This change is