Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optionally remove Status handler_id prefix. #579

Closed
wants to merge 5 commits into from

Conversation

samgiles
Copy link

@samgiles samgiles commented Nov 12, 2020

What do these changes do?

This ports a PR that was originally raised (zalando-incubator/kopf#320) on the Zalando repository.
Thanks @lack!

Adding an optional parameter to the decorators for resource change handlers that allows the return value to be stored directly onto the status key on the resource in question.

Description

Implements the proposal in issue #319.

Issues/PRs

#319

Type of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • The code addresses only the mentioned problem, and this problem only
  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt

I can't take credit for the code itself since it was already written here: zalando-incubator/kopf#320

I'm not sure if we've covered all the cases here, would be good to know if I've missed something.

spec:
...
status:
field: value
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: What will happen with return "hello" or return 123 or other scalars? What about return ["abc", "def"]?

Comment on lines +1 to +3
=======
Updating Status
=======
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
=======
Updating Status
=======
===============
Updating Status
===============

=======

Kopf will take the return value of all state-changing handlers (even
sub-handlers) and add them to the ``Status`` of the corresponding Kuberenetes
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sub-handlers) and add them to the ``Status`` of the corresponding Kuberenetes
sub-handlers) and add them to the ``.status`` stanza of the corresponding Kubernetes


@kopf.on.create('zalando.org', 'v1', 'kopfexamples')
def my_handler(spec, **_):
return {'field': 'value}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please wrap all code blocks here (before and after this comment) to their specific language highlights?

This is one of the following instead of :::

.. code-block:: yaml
.. code-block:: python

I'm highly uncertain if there is any kind of automatic language detection in SphinxDocs.

else:
patch.setdefault('status', {})[handler_id] = copy.deepcopy(outcome.result)
if hasattr(outcome.handler, 'status_prefix') and not outcome.handler.status_prefix:
patch.setdefault('status', {})[copy.deepcopy(outcome.result)] = {}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: I'm quite not sure what does this line mean. Can you please clarify the intentions? It looks like the result is used as the key, not as the value.

@@ -145,6 +145,7 @@ def resume( # lgtm[py/similar-function]
labels: Optional[filters.MetaFilter] = None,
annotations: Optional[filters.MetaFilter] = None,
when: Optional[callbacks.WhenFilterFn] = None,
status_prefix: bool = True,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: Generally, as a rule of thumb, all handler options are designed so that the default value is None and evaluates to falseness. Even for booleans (see optional or deleted in some handlers nearby — it is Optional[bool]). I would prefer that all user-facing DSL features follow this convention in the future.

Can you please negate the option and its meaning? (I am not sure what would be the better wording to describe this: flat status? direct status? raw status? result_to_status? something else?)

@nolar
Copy link
Owner

nolar commented Nov 15, 2020

On a side note, I thought on a similar feature in relation to a completely different purpose (cross-resource handlers). One idea that I had is to separate the result delivery from the handler declaration completely — via @kopf.to.... decorators of different kind. Your goal could be achieved with:

@kopf.on.create('zalando.org', 'v1', 'kopfexamples')
@kopf.on.resume('zalando.org', 'v1', 'kopfexamples')
@kopf.to.status(id='')
def fn(**_):
    return {'hello': 'world'}

The absence of @kopf.to decorators, for backward compatibility, would be equivalent to @kopf.to.status(), with id equal to the function name — same as the id option for the handlers now.

Why so? In the long-term, this would allow to extend it with multiple delivery targets, including those not even in this same resource:

@kopf.on.create('zalando.org', 'v1', 'kopfexamples')
@kopf.on.resume('zalando.org', 'v1', 'kopfexamples')
@kopf.to.status(id='')
@kopf.to.owners(field='status.children.{}')  # via ownerReferences, to all of them if there are many
@kopf.to.owners(kind='KopfAnotherExample')  # via ownerReferences, to only a specific kind
@kopf.to.related(kind='KopfAnotherExample', via=...)  # matching criteria are uncertain here (yet)
def fn(**_):
    return {'hello': 'world'}

Usage of decorators allows flexibility with some options for each delivery target, and multiple targets at once.

The exact semantics is not yet fully clarified, so consider the snippets above just as ideas.

Implementation-wide, I thought about storing the "to" values directly on a function as a hidden attribute (e.g. _kopf_to_targets=[] as a list of these target definitions), so that all handlers (create/resume in this example) would deliver to all declared targets. It means that the target is not a property of the handler, but of the function.

While all these types of targets are not in the nearest scope for implementation, is there a way to consider using @kopf.to.status() in this implementation? How much more difficult would it be?

It is not a problem if this is not implemented now. There is a clear visible way of ensuring backward compatibility with the handlers' option when it is implemented (with some nuances for double-handling functions (e.g. create+resum) with different setup). So, if the complexity is too high, it is better to postpone this solution for later, and implement it as it is done in this PR via handlers' properties.

Base automatically changed from master to main March 10, 2021 19:45
@caffeinism
Copy link

any plans now?

@nolar
Copy link
Owner

nolar commented Sep 3, 2021

On the @kopf.to. syntax — no, there are no current plans, the priorities are different.

On this PR — I will close it for now: there are too many conflicts, they are difficult to resolve, and some issues from the comments are not solved (and I'm not sure how to solve them properly on the user-story level: e.g., return 123).

As a workaround to achieve the desired effect, use the patch kwarg and populate it as needed instead of return'ing values from the handler (the snippet from #319):

import kopf

@kopf.on.create("zalando.org", "v1", "kopfexamples")
async def created(spec, patch, **kwargs):
    patch.status["result"] = "Created"  # instead of: return {"result": "Created"}

@kopf.on.update("zalando.org", "v1", "kopfexamples")
async def updated(spec, patch, **kwargs):
    patch.status["result"] = "Updated"

With this tool in hand, adding the extra flag does not look significantly more beneficial, but adds complexity and maintenance burden.

PS: Extracted to #827.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants