Add in-cluster config support #32
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
What's the plan for enabling automated testing (unit/integration/e2e)? There's only so much a review of code can do, and I'd rather be reviewing code + tests, and have those tests run automatically. |
|
@marun Thanks for demanding test from a lazy engineer here :) It is in much better shape now. |
mbohlool
force-pushed
the
o4
branch
2 times, most recently
from
e9568d2
to
4b1c41f
Compare
|
@marun would you please take another look at this? I want to create a release after this PR. |
| def _join_host_port(host, port): | ||
| """Adapted golang's net.JoinHostPort""" | ||
| if ':' in host or '%' in host: | ||
| return "[%s]:%s" % (host, str(port)) |
There was a problem hiding this comment.
(No action required)
Consider avoiding explicitly calling str() (i.e. '%s' % 42 works just fine).
Also, consider the following alternate implementation:
template = '%s:%s'
# Where possible, assign a conditional statement to a variable and name it descriptively.
# This can reduce the need for comments, which cost more to maintain than code that is self-documenting.
host_requires_bracketing = ':' in host or '%' in host
if host_requires_bracketing:
template = '[%s]:%s'
return template % (host, port)| if not os.path.isfile(self._token_filename): | ||
| raise ConfigException("Service token file does not exists.") | ||
|
|
||
| with open(self._token_filename, 'r') as f: |
There was a problem hiding this comment.
(No action required)
'r' is the default and can be omitted.
| self._environ[self._port_env_name])) | ||
|
|
||
| if not os.path.isfile(self._token_filename): | ||
| raise ConfigException("Service token file does not exists.") |
There was a problem hiding this comment.
Consider separating reading of cluster config from modifying configuration to ensure it is only modified once configuration has been validated. This separation of concerns will also allow simplify testing by allowing the read values to be checked without involving configuration:
# Consider using namedtuple instead of a dict for simple types
ClusterConfig = collections.NamedTuple('ClusterConfig', ['host', 'token'...])
def load_incluster_config(environ=os.environ):
conf = read_incluster_config(environ=environ)
set_incluster_config(conf)
def read_incluster_config(environ=os.environ):
# Consider checking the value of env vars rather than the presence of the name to protect against empty vars
service_host = environ.get(_SERVICE_HOST_ENV_NAME)
if not service_host:
raise ConfigException("Service host not set.")
...
host = 'https://%s' % (_join_host_port(service_host, service_port)
return ClusterConfig(host, token...)
def set_incluster_config(conf):
configuration.host = conf.host
...This will also avoid the problem of modifying configuration in one unit test and accidentally having that modification affect the behavior of a different test that relies on its state. Due to configuration being a singleton, any changes to its default state must be reverted on test teardown.
There was a problem hiding this comment.
I've used the InClusterConfigLoader to store the values and separate load and set methods.
|
|
||
| from .incluster_config import ConfigException, InClusterConfigLoader | ||
|
|
||
| _TEST_PORT_ENV_NAME = "port" |
There was a problem hiding this comment.
Consider reusing the names of the env vars defined as constants in the module under test (e.g. _SERVICE_HOST_ENV_NAME) rather than defining new names in this test module. Since the environ dict is passed to the function under test, there is no need to vary the names of the env vars and the names can be fixed across runtime and test.
|
@marun I am going to discuss ci and e2e with kubernetes e2e test folks in a separate thread/issue. |
|
@marun, I've addressed all of your comments. As all of them were optional, I assume this PR is look good. Please apply LGTM next time if your comments are all optional. Thanks for the review. |
mbohlool
added
the
lgtm
| raise ConfigException("Service token file does not exists.") | ||
|
|
||
| with open(self._token_filename) as f: | ||
| self.token = f.read() |
There was a problem hiding this comment.
Missed these comments, addressed them in #50
| self._set_config() | ||
|
|
||
| def _load_config(self): | ||
| if (self._host_env_name not in self._environ or |
There was a problem hiding this comment.
What about the case where either of the env vars are defined but empty (e.g. KUBERNETES_SERVICE_HOST=)?
There was a problem hiding this comment.
Missed these comments, addressed them in #50
enables client-python to be used inside a kubernetes cluster.
Fixes #26