-
Notifications
You must be signed in to change notification settings - Fork 185
Support false values in configuration file #161
Conversation
Welcome @ganchurin! |
Codecov Report
@@ Coverage Diff @@
## master #161 +/- ##
=======================================
Coverage 93.56% 93.56%
=======================================
Files 13 13
Lines 1384 1384
=======================================
Hits 1295 1295
Misses 89 89
Continue to review full report at Codecov.
|
/assign @yliaog |
@@ -564,6 +564,7 @@ class TestKubeConfigLoader(BaseTestCase): | |||
"server": TEST_SSL_HOST, | |||
"certificate-authority-data": | |||
TEST_CERTIFICATE_AUTH_BASE64, | |||
"insecure-skip-tls-verify": False, |
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.
s/False/"false" in consistent with the test case below
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.
@roycaihw Thanks for the suggestion! I've just tried to replace False
with "false"
and faced an issue - the unit test fails. I think the cause is in how "insecure-skip-tls-verify" property is handled in kube_config.py
(https://github.com/kubernetes-client/python-base/blob/master/config/kube_config.py#L443). The code by the link casts a string to a boolean. Both "true"
and "false"
are casted to True
. What do you think if I replace "true"
with True
in the older test?
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.
"true"
and "false"
are the original values in kubeconfig yaml files (e.g. insecure-skip-tls-verify: false
). The test cases are testing values stored in the KubeConfigLoader
object.
Could you verify what's the behavior in the yaml loader? Does it load "false"
into a boolean False
, or does it keep the string value "false"
?
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 verified the behavior on the configuration file which has the following lines:
clusters:
- cluster:
insecure-skip-tls-verify: false
The following code snippet was used for the verification:
import yaml
with open(<PATH_TO_CONFIG>) as f:
config = yaml.safe_load(f)
print config (**breakpoint was set here**)
Debug showed the following value for the property:
'insecure-skip-tls-verify' (4523129968) = {bool} False
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.
Thanks. Let's use True
and False
in the test cases then
/lgtm Thanks @ganchurin! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ganchurin, roycaihw 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 |
/retest |
@ganchurin: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
@roycaihw Could you please help me a bit? Some tests have failed after my update to the branch. I'm not sure that they relate to the scope of work. And I can't restart test run. Thanks in advance! |
/ok-to-test |
The Travis CI is showing a ton of 404 and other errors. Try looking at the log - it is available. |
/retest |
@micw523 Thank you! I've taken a look. I've noticed that the same tests fail in the latest build of "kubernetes-client/python" repository: https://travis-ci.org/kubernetes-client/python/builds/587650221 As for my pull request, previously the pull request had passed successfully. The single difference between successful and failed attempts is one line in unit tests that works fine for me locally. Thus I tend thinking that the issue is in "kubernetes-client/python" repository. |
The most recent build in k8s-python passed; I'm restarting your build to see if the test fail is a flake. |
@micw523 Sorry if I'm wrong - I was looking for builds here: https://travis-ci.org/kubernetes-client/python/builds. And the latest build is red. |
Test failure tracked in kubernetes-client/python#963. Let's fix the CI first and then merge this PR |
The purpose of this changes is to fix the issue: kubernetes-client/python#954