From 95a6db3b312bc1bf5f61b78b4391d9e5b4f2baf6 Mon Sep 17 00:00:00 2001 From: Sergey Vasilyev Date: Mon, 27 Jan 2020 18:04:41 +0100 Subject: [PATCH 1/5] Improve code style on LGTM's warnings & few recommendations --- examples/04-events/example.py | 2 +- examples/13-hooks/example.py | 3 +-- kopf/clients/auth.py | 8 +++----- kopf/reactor/running.py | 3 +-- 4 files changed, 6 insertions(+), 10 deletions(-) diff --git a/examples/04-events/example.py b/examples/04-events/example.py index f4666ed6..945f00f8 100644 --- a/examples/04-events/example.py +++ b/examples/04-events/example.py @@ -16,5 +16,5 @@ def create_fn(body, **kwargs): try: raise RuntimeError("Exception text.") - except: + except Exception: kopf.exception(body, reason="SomeReason", message="Some exception:") diff --git a/examples/13-hooks/example.py b/examples/13-hooks/example.py index c7be4ee2..ce80567b 100644 --- a/examples/13-hooks/example.py +++ b/examples/13-hooks/example.py @@ -86,5 +86,4 @@ async def _task_fn(logger, shouldstop: asyncio.Event) -> NoReturn: while not shouldstop.is_set(): await asyncio.sleep(random.randint(1, 10)) logger.info("Served by the background task.") - else: - logger.info("Serving is finished by request.") + logger.info("Serving is finished by request.") diff --git a/kopf/clients/auth.py b/kopf/clients/auth.py index ada72289..4e113573 100644 --- a/kopf/clients/auth.py +++ b/kopf/clients/auth.py @@ -49,8 +49,7 @@ async def wrapper(*args: Any, **kwargs: Any) -> Any: await vault.invalidate(key, exc=e) else: raise - else: - raise credentials.LoginError("Ran out of connection credentials.") + raise credentials.LoginError("Ran out of connection credentials.") return cast(_F, wrapper) @@ -79,14 +78,13 @@ async def wrapper(*args: Any, **kwargs: Any) -> Any: try: async for item in fn(*args, **kwargs, context=context): yield item - break # out of credentials cycle (instead of `return`) + return except aiohttp.ClientResponseError as e: if e.status == 401: await vault.invalidate(key, exc=e) else: raise - else: - raise credentials.LoginError("Ran out of connection credentials.") + raise credentials.LoginError("Ran out of connection credentials.") return cast(_F, wrapper) diff --git a/kopf/reactor/running.py b/kopf/reactor/running.py index c0016c5a..97a72fcf 100644 --- a/kopf/reactor/running.py +++ b/kopf/reactor/running.py @@ -72,8 +72,7 @@ def login( for outcome in e.outcomes.values(): if isinstance(outcome.exception, credentials.LoginError): raise outcome.exception - else: - raise + raise def run( From 6207b3c87b7bf0783d219d1cfac5362c428b7782 Mon Sep 17 00:00:00 2001 From: Sergey Vasilyev Date: Mon, 27 Jan 2020 20:44:00 +0100 Subject: [PATCH 2/5] Rewrite the task orchestration for simpler code, no useless vars The logic is supposed to be exactly the same, just all the try-except-else are rewritten differently (more straightforward). This allows to reindent the code better (shorter lines), remove the unused variables (to make the linters happy), while keeping is visually symmetric. --- kopf/reactor/running.py | 44 +++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/kopf/reactor/running.py b/kopf/reactor/running.py index 97a72fcf..9fa767cc 100644 --- a/kopf/reactor/running.py +++ b/kopf/reactor/running.py @@ -308,32 +308,34 @@ async def run_tasks( Only the tasks that existed before the operator startup are ignored (for example, those that spawned the operator itself). """ + + # Run the infinite tasks until one of them fails/exits (they never exit normally). + # If the operator is cancelled, propagate the cancellation to all the sub-tasks. + # There is no graceful period: cancel as soon as possible, but allow them to finish. try: - # Run the infinite tasks until one of them fails/exits (they never exit normally). root_done, root_pending = await _wait(root_tasks, return_when=asyncio.FIRST_COMPLETED) except asyncio.CancelledError: - # If the operator is cancelled, propagate the cancellation to all the sub-tasks. - # There is no graceful period: cancel as soon as possible, but allow them to finish. - root_cancelled, root_left = await _stop(root_tasks, title="Root", cancelled=True) + await _stop(root_tasks, title="Root", cancelled=True) hung_tasks = await _all_tasks(ignored=ignored) - hung_cancelled, hung_left = await _stop(hung_tasks, title="Hung", cancelled=True) + await _stop(hung_tasks, title="Hung", cancelled=True) raise - else: - # If the operator is intact, but one of the root tasks has exited (successfully or not), - # cancel all the remaining root tasks, and gracefully exit other spawned sub-tasks. - root_cancelled, root_left = await _stop(root_pending, title="Root", cancelled=False) - hung_tasks = await _all_tasks(ignored=ignored) - try: - # After the root tasks are all gone, cancel any spawned sub-tasks (e.g. handlers). - # TODO: assumption! the loop is not fully ours! find a way to cancel our spawned tasks. - hung_done, hung_pending = await _wait(hung_tasks, timeout=5.0) - except asyncio.CancelledError: - # If the operator is cancelled, propagate the cancellation to all the sub-tasks. - hung_cancelled, hung_left = await _stop(hung_tasks, title="Hung", cancelled=True) - raise - else: - # If the operator is intact, but the timeout is reached, forcely cancel the sub-tasks. - hung_cancelled, hung_left = await _stop(hung_pending, title="Hung", cancelled=False) + + # If the operator is intact, but one of the root tasks has exited (successfully or not), + # cancel all the remaining root tasks, and gracefully exit other spawned sub-tasks. + root_cancelled, _ = await _stop(root_pending, title="Root", cancelled=False) + + # After the root tasks are all gone, cancel any spawned sub-tasks (e.g. handlers). + # If the operator is cancelled, propagate the cancellation to all the sub-tasks. + # TODO: an assumption! the loop is not fully ours! find a way to cancel only our spawned tasks. + hung_tasks = await _all_tasks(ignored=ignored) + try: + hung_done, hung_pending = await _wait(hung_tasks, timeout=5.0) + except asyncio.CancelledError: + await _stop(hung_tasks, title="Hung", cancelled=True) + raise + + # If the operator is intact, but the timeout is reached, forcely cancel the sub-tasks. + hung_cancelled, _ = await _stop(hung_pending, title="Hung", cancelled=False) # If succeeded or if cancellation is silenced, re-raise from failed tasks (if any). await _reraise(root_done | root_cancelled | hung_done | hung_cancelled) From 2702e34dd2b09f0ce2c795fb260fd22e0b222d66 Mon Sep 17 00:00:00 2001 From: Sergey Vasilyev Date: Mon, 27 Jan 2020 21:26:49 +0100 Subject: [PATCH 3/5] Use the actual registry as of when decorators are applied, not created This also should resolve some code linting problems in advance. --- kopf/on.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/kopf/on.py b/kopf/on.py index e76b9050..779131d2 100644 --- a/kopf/on.py +++ b/kopf/on.py @@ -36,8 +36,8 @@ def startup( cooldown: Optional[float] = None, # deprecated, use `backoff` registry: Optional[registries.OperatorRegistry] = None, ) -> ActivityHandlerDecorator: - actual_registry = registry if registry is not None else registries.get_default_registry() def decorator(fn: callbacks.ActivityHandlerFn) -> callbacks.ActivityHandlerFn: + actual_registry = registry if registry is not None else registries.get_default_registry() return actual_registry.register_activity_handler( fn=fn, id=id, errors=errors, timeout=timeout, retries=retries, backoff=backoff, cooldown=cooldown, @@ -56,8 +56,8 @@ def cleanup( cooldown: Optional[float] = None, # deprecated, use `backoff` registry: Optional[registries.OperatorRegistry] = None, ) -> ActivityHandlerDecorator: - actual_registry = registry if registry is not None else registries.get_default_registry() def decorator(fn: callbacks.ActivityHandlerFn) -> callbacks.ActivityHandlerFn: + actual_registry = registry if registry is not None else registries.get_default_registry() return actual_registry.register_activity_handler( fn=fn, id=id, errors=errors, timeout=timeout, retries=retries, backoff=backoff, cooldown=cooldown, @@ -77,8 +77,8 @@ def login( registry: Optional[registries.OperatorRegistry] = None, ) -> ActivityHandlerDecorator: """ ``@kopf.on.login()`` handler for custom (re-)authentication. """ - actual_registry = registry if registry is not None else registries.get_default_registry() def decorator(fn: callbacks.ActivityHandlerFn) -> callbacks.ActivityHandlerFn: + actual_registry = registry if registry is not None else registries.get_default_registry() return actual_registry.register_activity_handler( fn=fn, id=id, errors=errors, timeout=timeout, retries=retries, backoff=backoff, cooldown=cooldown, @@ -98,8 +98,8 @@ def probe( registry: Optional[registries.OperatorRegistry] = None, ) -> ActivityHandlerDecorator: """ ``@kopf.on.probe()`` handler for arbitrary liveness metrics. """ - actual_registry = registry if registry is not None else registries.get_default_registry() def decorator(fn: callbacks.ActivityHandlerFn) -> callbacks.ActivityHandlerFn: + actual_registry = registry if registry is not None else registries.get_default_registry() return actual_registry.register_activity_handler( fn=fn, id=id, errors=errors, timeout=timeout, retries=retries, backoff=backoff, cooldown=cooldown, @@ -124,8 +124,8 @@ def resume( when: Optional[callbacks.WhenHandlerFn] = None, ) -> ResourceHandlerDecorator: """ ``@kopf.on.resume()`` handler for the object resuming on operator (re)start. """ - actual_registry = registry if registry is not None else registries.get_default_registry() def decorator(fn: callbacks.ResourceHandlerFn) -> callbacks.ResourceHandlerFn: + actual_registry = registry if registry is not None else registries.get_default_registry() return actual_registry.register_resource_changing_handler( group=group, version=version, plural=plural, reason=None, initial=True, deleted=deleted, id=id, @@ -150,8 +150,8 @@ def create( when: Optional[callbacks.WhenHandlerFn] = None, ) -> ResourceHandlerDecorator: """ ``@kopf.on.create()`` handler for the object creation. """ - actual_registry = registry if registry is not None else registries.get_default_registry() def decorator(fn: callbacks.ResourceHandlerFn) -> callbacks.ResourceHandlerFn: + actual_registry = registry if registry is not None else registries.get_default_registry() return actual_registry.register_resource_changing_handler( group=group, version=version, plural=plural, reason=causation.Reason.CREATE, id=id, @@ -176,8 +176,8 @@ def update( when: Optional[callbacks.WhenHandlerFn] = None, ) -> ResourceHandlerDecorator: """ ``@kopf.on.update()`` handler for the object update or change. """ - actual_registry = registry if registry is not None else registries.get_default_registry() def decorator(fn: callbacks.ResourceHandlerFn) -> callbacks.ResourceHandlerFn: + actual_registry = registry if registry is not None else registries.get_default_registry() return actual_registry.register_resource_changing_handler( group=group, version=version, plural=plural, reason=causation.Reason.UPDATE, id=id, @@ -203,8 +203,8 @@ def delete( when: Optional[callbacks.WhenHandlerFn] = None, ) -> ResourceHandlerDecorator: """ ``@kopf.on.delete()`` handler for the object deletion. """ - actual_registry = registry if registry is not None else registries.get_default_registry() def decorator(fn: callbacks.ResourceHandlerFn) -> callbacks.ResourceHandlerFn: + actual_registry = registry if registry is not None else registries.get_default_registry() return actual_registry.register_resource_changing_handler( group=group, version=version, plural=plural, reason=causation.Reason.DELETE, id=id, @@ -231,8 +231,8 @@ def field( when: Optional[callbacks.WhenHandlerFn] = None, ) -> ResourceHandlerDecorator: """ ``@kopf.on.field()`` handler for the individual field changes. """ - actual_registry = registry if registry is not None else registries.get_default_registry() def decorator(fn: callbacks.ResourceHandlerFn) -> callbacks.ResourceHandlerFn: + actual_registry = registry if registry is not None else registries.get_default_registry() return actual_registry.register_resource_changing_handler( group=group, version=version, plural=plural, reason=None, field=field, id=id, @@ -252,8 +252,8 @@ def event( when: Optional[callbacks.WhenHandlerFn] = None, ) -> ResourceHandlerDecorator: """ ``@kopf.on.event()`` handler for the silent spies on the events. """ - actual_registry = registry if registry is not None else registries.get_default_registry() def decorator(fn: callbacks.ResourceHandlerFn) -> callbacks.ResourceHandlerFn: + actual_registry = registry if registry is not None else registries.get_default_registry() return actual_registry.register_resource_watching_handler( group=group, version=version, plural=plural, id=id, fn=fn, labels=labels, annotations=annotations, when=when, @@ -302,8 +302,8 @@ def create_task(*, spec, task=task, **kwargs): Note: ``task=task`` is needed to freeze the closure variable, so that every create function will have its own value, not the latest in the for-cycle. """ - actual_registry = registry if registry is not None else handling.subregistry_var.get() def decorator(fn: callbacks.ResourceHandlerFn) -> callbacks.ResourceHandlerFn: + actual_registry = registry if registry is not None else handling.subregistry_var.get() return actual_registry.register( id=id, fn=fn, errors=errors, timeout=timeout, retries=retries, backoff=backoff, cooldown=cooldown, From 74ee728fca76712bfaccdd12749a7ecbdb4e98d4 Mon Sep 17 00:00:00 2001 From: Sergey Vasilyev Date: Tue, 28 Jan 2020 12:49:58 +0100 Subject: [PATCH 4/5] Add LGTM badges to README --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index d60659ea..02e5c92b 100644 --- a/README.md +++ b/README.md @@ -3,6 +3,8 @@ [![Build Status](https://travis-ci.org/zalando-incubator/kopf.svg?branch=master)](https://travis-ci.org/zalando-incubator/kopf) [![codecov](https://codecov.io/gh/zalando-incubator/kopf/branch/master/graph/badge.svg)](https://codecov.io/gh/zalando-incubator/kopf) [![Coverage Status](https://coveralls.io/repos/github/zalando-incubator/kopf/badge.svg?branch=master)](https://coveralls.io/github/zalando-incubator/kopf?branch=master) +[![Total alerts](https://img.shields.io/lgtm/alerts/g/zalando-incubator/kopf.svg?logo=lgtm&logoWidth=18)](https://lgtm.com/projects/g/zalando-incubator/kopf/alerts/) +[![Language grade: Python](https://img.shields.io/lgtm/grade/python/g/zalando-incubator/kopf.svg?logo=lgtm&logoWidth=18)](https://lgtm.com/projects/g/zalando-incubator/kopf/context:python) **Kopf** —Kubernetes Operator Pythonic Framework— is a framework and a library to make Kubernetes operators development easier, just in few lines of Python code. From 0074b96a165ad846dde0a8aa2427d3a80ef77cbc Mon Sep 17 00:00:00 2001 From: Sergey Vasilyev Date: Thu, 30 Jan 2020 11:58:42 +0100 Subject: [PATCH 5/5] Disable LGTM's alerts on `kopf.on` code duplication `kopf.on` decorators are intentionally duplicated for better IDE integrations and code hinting. --- kopf/on.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/kopf/on.py b/kopf/on.py index 779131d2..9aa4cfe4 100644 --- a/kopf/on.py +++ b/kopf/on.py @@ -26,7 +26,7 @@ def creation_handler(**kwargs): ActivityHandlerDecorator = Callable[[callbacks.ActivityHandlerFn], callbacks.ActivityHandlerFn] -def startup( +def startup( # lgtm[py/similar-function] *, id: Optional[str] = None, errors: Optional[errors_.ErrorsMode] = None, @@ -46,7 +46,7 @@ def decorator(fn: callbacks.ActivityHandlerFn) -> callbacks.ActivityHandlerFn: return decorator -def cleanup( +def cleanup( # lgtm[py/similar-function] *, id: Optional[str] = None, errors: Optional[errors_.ErrorsMode] = None, @@ -66,7 +66,7 @@ def decorator(fn: callbacks.ActivityHandlerFn) -> callbacks.ActivityHandlerFn: return decorator -def login( +def login( # lgtm[py/similar-function] *, id: Optional[str] = None, errors: Optional[errors_.ErrorsMode] = None, @@ -87,7 +87,7 @@ def decorator(fn: callbacks.ActivityHandlerFn) -> callbacks.ActivityHandlerFn: return decorator -def probe( +def probe( # lgtm[py/similar-function] *, id: Optional[str] = None, errors: Optional[errors_.ErrorsMode] = None, @@ -108,7 +108,7 @@ def decorator(fn: callbacks.ActivityHandlerFn) -> callbacks.ActivityHandlerFn: return decorator -def resume( +def resume( # lgtm[py/similar-function] group: str, version: str, plural: str, *, id: Optional[str] = None, @@ -135,7 +135,7 @@ def decorator(fn: callbacks.ResourceHandlerFn) -> callbacks.ResourceHandlerFn: return decorator -def create( +def create( # lgtm[py/similar-function] group: str, version: str, plural: str, *, id: Optional[str] = None, @@ -161,7 +161,7 @@ def decorator(fn: callbacks.ResourceHandlerFn) -> callbacks.ResourceHandlerFn: return decorator -def update( +def update( # lgtm[py/similar-function] group: str, version: str, plural: str, *, id: Optional[str] = None, @@ -187,7 +187,7 @@ def decorator(fn: callbacks.ResourceHandlerFn) -> callbacks.ResourceHandlerFn: return decorator -def delete( +def delete( # lgtm[py/similar-function] group: str, version: str, plural: str, *, id: Optional[str] = None, @@ -215,7 +215,7 @@ def decorator(fn: callbacks.ResourceHandlerFn) -> callbacks.ResourceHandlerFn: return decorator -def field( +def field( # lgtm[py/similar-function] group: str, version: str, plural: str, field: dicts.FieldSpec, *, @@ -242,7 +242,7 @@ def decorator(fn: callbacks.ResourceHandlerFn) -> callbacks.ResourceHandlerFn: return decorator -def event( +def event( # lgtm[py/similar-function] group: str, version: str, plural: str, *, id: Optional[str] = None, @@ -263,7 +263,7 @@ def decorator(fn: callbacks.ResourceHandlerFn) -> callbacks.ResourceHandlerFn: # TODO: find a better name: `@kopf.on.this` is confusing and does not fully # TODO: match with the `@kopf.on.{cause}` pattern, where cause is create/update/delete. -def this( +def this( # lgtm[py/similar-function] *, id: Optional[str] = None, errors: Optional[errors_.ErrorsMode] = None, @@ -311,7 +311,7 @@ def decorator(fn: callbacks.ResourceHandlerFn) -> callbacks.ResourceHandlerFn: return decorator -def register( +def register( # lgtm[py/similar-function] fn: callbacks.ResourceHandlerFn, *, id: Optional[str] = None,