Kube config loader improvement #50
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
| if not contexts: | ||
| print("Cannot find any context in kube-config file.") | ||
| return | ||
| for i in range(len(contexts)): |
There was a problem hiding this comment.
Why don't you deal with the context names directly ? something like
context_names =[]
for context in contexts:
context_names.append(context['name'])
context_name = input('enter context name')
if context_name in context_names:
config.load_kube_config(os.environ["HOME"] + '/.kube/config', context_name)
else:
return "wrong name, try again"
There was a problem hiding this comment.
The reason is a deleted part of the example that show hostname of the selected context. But even for that, it can be done easier. I will fix the example tomorrow. Thanks.
There was a problem hiding this comment.
(No action required) Consider enumerating the list items rather than indexing into the list:
for i, context in enumerate(contexts):
...
| if not contexts: | ||
| print("Cannot find any context in kube-config file.") | ||
| return | ||
| for i in range(len(contexts)): |
There was a problem hiding this comment.
(No action required) Consider enumerating the list items rather than indexing into the list:
for i, context in enumerate(contexts):
...| v1 = client.CoreV1Api() | ||
| print("Listing pods with their IPs:") | ||
| ret = v1.list_pod_for_all_namespaces(watch=False) | ||
| for i in ret.items: |
There was a problem hiding this comment.
(No action required) Consider avoiding the use of single-character variable names for non-index variables.
| # limitations under the License. | ||
|
|
||
|
|
||
| class ConfigException(Exception): |
There was a problem hiding this comment.
(No action required) Why is this necessary? Python exceptions have similar behavior by default. Unless there's a specific case that requires more rigor, I'd suggest YAGNI (you ain't gonna need it).
>>> str(Exception('foo'))
'foo'There was a problem hiding this comment.
Are you talking about the whole class or the message part? I agree about the message part, but the whole ConfigException class let use separate config related exception from other type of exceptions. It gives caller more control on what to do if it failed. I will, however, get rid of the message. I've added it because python3 does not have a message field (at least directly accessible message field) but I can just use str(*).
There was a problem hiding this comment.
The class is fine, just the message part.
| _TEST_ENVIRON = {_SERVICE_HOST_ENV_NAME: _TEST_HOST, | ||
| _SERVICE_PORT_ENV_NAME: _TEST_PORT} | ||
| _SERVICE_PORT_ENV_NAME: _TEST_PORT, |
There was a problem hiding this comment.
(No action required) Why is it necessary to be able to vary the name of the env var between runtime and test? I get varying the filenames since that enables testing isolation, but the env dict can be isolated without varying the env var names.
| @@ -57,7 +60,7 @@ def get_test_loader( | |||
| if not token_filename: | |||
| token_filename = self._create_file_with_temp_content(_TEST_TOKEN) | |||
| if not cert_filename: | |||
| cert_filename = self._create_file_with_temp_content() | |||
| cert_filename = self._create_file_with_temp_content(_TEST_CERT) | |||
There was a problem hiding this comment.
(No action required) Consider defaulting content to _TEST_CERT instead of "" since that's the more common case.
There was a problem hiding this comment.
I agree that numbers here suggest _TEST_CERT is more popular (3 against 2) but having a CERT being default for a function that does not have anything to do with certificates does not feel right. As the usage numbers of these two values are close I would prefer having empty string as default value (I feel right to have things like empty string as default value unless the default value means something to the behaviour of the function).
| return name | ||
|
|
||
|
|
||
| class TestFileOrData(TestRoot): |
There was a problem hiding this comment.
(No action required) Consider testing _create_temp_file_with_content directly rather than validating only through FileOrData.
There was a problem hiding this comment.
added test_create_temp_file_with_content. I've also simplified create_temp_file_with_content method and moved base64 out of it.
| message_part in e.message, "'%s' should be in '%s'" % | ||
| (message_part, e.message)) | ||
|
|
||
| def test_invalid_key(self): |
There was a problem hiding this comment.
The name of this test doesn't do a good job of communicating its intent. Consider renaming to something more descriptive.
Also, unit testing best practice is to make as few assertions as possible in a given test, and ideally each code path would get its own test.
| def test_invalid_key(self): | ||
| node = ConfigNode("test_obj", self.test_obj) | ||
| self.expect_exception(lambda: node['non-existance-key'], | ||
| "Expect key non-existance-key in test_obj") |
There was a problem hiding this comment.
Duplicating error messages in testing makes for brittle tests. If testing is comprehensive, it should be enough to isolate the failure condition:
def setUp(self):
self.node = ConfigNode("test_obj", self.test_obj)
def test_get_with_name_fails_if_value_not_a_list(self):
self.node._value = None
with self.assertRaises(ConfigException):
self.node.get_with_name('')There was a problem hiding this comment.
I agree generally, but here the purpose of ConfigNode is to have meaningful error messages. Specially full path of the config node that has the problem. maybe I can only check for the path itself? like in the above one, only "test_obj" or later ones for "test_obj/key3" etc.
There was a problem hiding this comment.
Given that the node name appears in most of the exceptions, I'm not sure what checking for it would buy you over the exception itself. If you want to validate anything beyond the fact that an exception is raised, consider using setting error codes on the exception that can be referred to via constants at both run and test time (e.g. IOException).
| self.assertEqual("test_obj_with_names[name=test_name3]", | ||
| node.get_with_name("test_name3").name) | ||
|
|
||
| def expect_exception(self, func, message_part): |
There was a problem hiding this comment.
There are better ways to do this:
def test_foo(self):
with self.assertRaises(MyException) as context:
foo()
self.assertIn('bar', context.exception)Reference: https://docs.python.org/2/library/unittest.html#assert-methods
| return "Config(%s\n)" % rep | ||
|
|
||
|
|
||
| class TestKubeConfigLoader(TestRoot): |
There was a problem hiding this comment.
(No action required) Validating entirely through load_and_set makes it hard to reason about which code paths are validated for a given test. That might be acceptable for an integration or e2e test, but for unit testing, consider ensuring comprehensive coverage of the lowest-level functions and only then consider the higher level testing required.
There was a problem hiding this comment.
I will add more unit tests for lower level functions, but keep these ones as it test the overall behaviour.
|
Addressed all of your comments. Please take another look and apply LGTM label if it looks good. Thanks. |
|
|
||
|
|
||
| def main(): | ||
| config_file = os.environ["HOME"] + '/.kube/config' |
There was a problem hiding this comment.
Can this be the "new" syntax like in #51 please?
|
|
||
| # Configs can be set in Configuration class directly or using helper | ||
| # utility | ||
| config.load_kube_config(os.environ["HOME"] + '/.kube/config', context_name) |
There was a problem hiding this comment.
same as above. Perhaps make a constant at the top? KUBE_CONFIG_FILE or something?
There was a problem hiding this comment.
I can just reuse config_file variable. Done.
| SERVICE_HOST_ENV_NAME = "KUBERNETES_SERVICE_HOST" | ||
| SERVICE_PORT_ENV_NAME = "KUBERNETES_SERVICE_PORT" | ||
| SERVICE_TOKEN_FILENAME = "/var/run/secrets/kubernetes.io/serviceaccount/token" | ||
| SERVICE_CERT_FILENAME = "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" |
There was a problem hiding this comment.
I know this is hardcoded on Linux containers, but is that the case on windows containers? I'm totally a Linux full time guy (typing from a Fedora 25 laptop), but these things seem relevant if k8s is going to run on windows, the client likely might as well.
There was a problem hiding this comment.
Good point. This is also hard-coded in k8s code. I would suggest you start a conversation on this on the main repo and if we changed the main repo, we can update this one too.
| for f in _temp_files: | ||
| os.remove(f) | ||
| for temp_file in _temp_files.values(): | ||
| os.remove(temp_file) |
There was a problem hiding this comment.
Spurious change, or just OCD? 😄
This is functionally 100% the same sans a better for loop variable name. The values() isn't necessary.
There was a problem hiding this comment.
_temp_files was array before and dict now, so I cannot do it without values() (it will give me the keys in case of dict).
There was a problem hiding this comment.
gah, I must have been smoking crack, you're entirely right here.
| return self._current_context.value | ||
|
|
||
|
|
||
| class ConfigNode: |
There was a problem hiding this comment.
If this is going to be used in python 2.7, can this be a new style class please?
|
Address @SEJeff comments too. PTAL. |
There was a problem hiding this comment.
A minor note regarding unnecessary public scoping for use in testing.
As an aside, I find it really difficult to re-review non-trivial PR's such as this one without a patch history. I would appreciate either fixup patches so I can see what changes (and then those fixups would be squashed before merge) or a tool like reviewable that keeps a history of all commit versions for the pr.
|
|
||
| def _join_host_port(host, port): | ||
|
|
||
| def join_host_port(host, port): |
There was a problem hiding this comment.
I'm assuming you removed the _ prefix because it is now imported by the test module. That isn't necessary - a leading underscore indicating 'private' is a good convention but one that unit tests do not have to respect. Unless you want to have to support this function for users outside this module, I suggest reverting the name change.
There was a problem hiding this comment.
I agree that tests should not respect that convention, but to be able to use this method outside of this module, one should import it in init. Because I saw that gateway exists and it is still private to module, I did not see any problem make it package access. I however have no strong opinion about it, so I will just revert this one, but we can discuss if init gateway is enough for being private inside a package or we need to always follow _ convention.
| @@ -22,151 +22,243 @@ | |||
| from kubernetes.client import configuration | |||
| from oauth2client.client import GoogleCredentials | |||
|
|
|||
| _temp_files = [] | |||
| from .incluster_config import ConfigException | |||
There was a problem hiding this comment.
Why isn't this imported from .config_exception? Consider avoiding relying on another module's imports.
There was a problem hiding this comment.
Good catch, fixed. (ConfigException was in incluster_config before, I moved it but apparently in python you can steal other modules import, I don't understand the benefit of that :)
|
|
||
|
|
||
| def _create_temp_file_with_content(content): | ||
| def create_temp_file_with_content(content): |
|
lgtm! |
|
LGTM |
- Refactor kube config loader to be able to test it - Add test for kube config loader
Current implementation of kube config loader, loads the default contexts. In this PR, it changed to load any context or list contexts. Also the code is refactored for better testability. kube config loader tests also added.
fixes #46