Move regular expression compilation to the top of the module in create_from_yaml

[abhiabhi94]

I was going through the codebase and was trying to understand it. I landed up at:

python/kubernetes/utils/create_from_yaml.py

Lines 142 to 178 in 6d64cf6

	def create_from_yaml_single_item(
	k8s_client, yml_object, verbose=False, **kwargs):
	group, _, version = yml_object["apiVersion"].partition("/")
	if version == "":
	version = group
	group = "core"
	# Take care for the case e.g. api_type is "apiextensions.k8s.io"
	# Only replace the last instance
	group = "".join(group.rsplit(".k8s.io", 1))
	# convert group name from DNS subdomain format to
	# python class name convention
	group = "".join(word.capitalize() for word in group.split('.'))
	fcn_to_call = "{0}{1}Api".format(group, version.capitalize())
	k8s_api = getattr(client, fcn_to_call)(k8s_client)
	# Replace CamelCased action_type into snake_case
	kind = yml_object["kind"]
	kind = re.sub('(.)([A-Z][a-z]+)', r'\1_\2', kind)
	kind = re.sub('([a-z0-9])([A-Z])', r'\1_\2', kind).lower()
	# Expect the user to create namespaced objects more often
	if hasattr(k8s_api, "create_namespaced_{0}".format(kind)):
	# Decide which namespace we are going to put the object in,
	# if any
	if "namespace" in yml_object["metadata"]:
	namespace = yml_object["metadata"]["namespace"]
	kwargs['namespace'] = namespace
	resp = getattr(k8s_api, "create_namespaced_{0}".format(kind))(
	body=yml_object, **kwargs)
	else:
	kwargs.pop('namespace', None)
	resp = getattr(k8s_api, "create_{0}".format(kind))(
	body=yml_object, **kwargs)
	if verbose:
	msg = "{0} created.".format(kind)
	if hasattr(resp, 'status'):
	msg += " status='{0}'".format(str(resp.status))
	print(msg)
	return resp

I think we can refactor this function by breaking down it into a couple of nested functions to reduce cognitive complexity and maybe distribute responsibilities.

Furthermore, I have this suggestion and would love to contribute if such a change is acceptable.

upper_followed_by_lower_re = re.compile('(.)([A-Z][a-z]+)')
lower_or_num_followed_by_upper_re = re.compile('([a-z0-9])([A-Z])')

def create_from_yaml_single_item(
        k8s_client, yml_object, verbose=False, **kwargs):

    def get_version_and_group(_api_version):
        _group, _, _version = _api_version.partition("/")
        if _version == "":
            _version = group
            _group = "core"
        return _version, _group

    def clean_group(_group):
        # Take care for the case e.g. api_type is "apiextensions.k8s.io"
        # Only replace the last instance
        _group = "".join(_group.rsplit(".k8s.io", 1))
        # convert group name from DNS subdomain format to
        # python class name convention
        return "".join(word.capitalize() for word in _group.split('.'))

    def clean_kind(_kind):
        # Replace CamelCased action_type into snake_case
        _kind = upper_followed_by_lower_re.sub(r'\1_\2', _kind)
        return lower_or_num_followed_by_upper_re.sub(r'\1_\2', _kind).lower()

    def get_response(_yml_obj, _k8s_api, _kind, **_kwargs):

        def get_namespaced_response(__yml_obj, __k8s_api, __kind, **__kwargs):
            __namespace = __yml_obj["metadata"]["namespace"]
            __kwargs['namespace'] = __namespace
            return getattr(__k8s_api, "create_namespaced_{0}".format(_kind_))(
                body=__yml_obj, **__kwargs)

        def get_non_namespaced_response(__yml_obj, __k8s_apis, __kind, **__kwargs):
            __kwargs.pop('namespace', None)
            return getattr(__k8s_api, "create_{0}".format(__kind))(
                body=__yml_obj, **__kwargs)

        # Expect the user to create namespaced objects more often
        if hasattr(_k8s_api, "create_namespaced_{0}"._format(kind)):
            # Decide which namespace we are going to put the object in,
            # if any
            return get_namespaced_response(_yml_obj, _k8s_api, _kind, **_kwargs)
        return get_non_namespaced_response(_yml_obj, _k8s_api, _kind, **_kwargs)


    def display_message(_kind, _resp, *, _verbose):
        if _verbose:
            _msg = "{0} created.".format(_kind)
            if hasattr(resp, 'status'):
                _msg += " status='{0}'".format(str(_resp.status))
            print(_msg)


    version, group = get_version_and_group(yml_object['apiVersion'])
    group = clean_group(group)

    fcn_to_call = "{0}{1}Api".format(group, version.capitalize())
    k8s_api = getattr(client, fcn_to_call)(k8s_client)
    kind = clean_kind(yml_object['kind'])
    resp = get_response(yml_object, k8s_api, kind, **kwargs)
    display_message(kind, resp, _verbose=verbose)
    return resp

Regarding the code:

• Prefixed _ in nested function variables so that they are there are lesser chances of facing scoping issues.
• Move the regular expression pattern to the top of the module as it can hopefully reduce execution time.

I believe a couple of other functions too can be broken down but just wanted to confirm if this is acceptable.

[roycaihw]

/assign @yliaog

[yliaog]

thanks for willing to help. i think functions are helpful when they are need to be called in multiple places, they do not help much in helping with readability. Please feel free to refactor the code with your other improvement ideas, e.g. Move the regular expression pattern to the top

[abhiabhi94]

i think functions are helpful when they are need to be called in multiple places, they do not help much in helping with readability

To someone who is not really well versed with the codebase, a function comprising around 15 lines might be a might hard to read v/s a 3-4 functions of 3-4 lines each.

For e.g.

def some_large_func(foo, bar):
    if foo:
        for i in range(2, foo):
            if foo % i == 0:
                break
        else:
            print('number is prime')

    if bar:
        for b in bar:
            # do some more manipulations for b


def some_large_fun(foo, bar):
    def is_prime(_f):
       for i in range(2, _f):
            if _f % i == 0:
                break
        else:
            print('number is prime')

    def process_bar(_b):
        # manipulate bar here

    if foo:
        is_prime(foo)
    if bar:
        process_bar(bar)

Also, it would be easier to debug(if and when required) if the responsibilities are distributed among different functions.

Still not convincing enough?

[yliaog]

in this case, functions serve as dividing the code into a few blocks, and give each block some extra documentation (input, output, name). my thinking is it's not very helpful to use 'real' functions to do these. They can be done with just blocks, and documentations.

[abhiabhi94]

one more thing that i forgot to mention, they was a potential boolean-trap in the function mentioned with verbose argument. I had removed in the suggestion above. would you like to remove the boolean-trap? it will have backward incompatibility.

[yliaog]

it's nice to remove boolean-trap, but introducing backward incompatibility is a concern. a quick search shows it is referenced e.g. http://danielhnyk.cz/programatically-spawn-kubernetes-resources-from-python/

[abhiabhi94]

So, if you agree, i can make a separate pull request for removing the boolean-trap, and it can be merged when a major version is upgraded.

[yliaog]

i'm not sure about the backward incompatibility part, we need the community input on whether it's worthwhile to make the breaking change. Feel free to send out a PR, we can ask the community to weigh in.