Skip to content
This repository has been archived by the owner on Sep 14, 2020. It is now read-only.

Extended syntax for filtering and accessing labels/annotations/patches/body parts #327

Merged
merged 15 commits into from
Mar 12, 2020

Conversation

nolar
Copy link
Contributor

@nolar nolar commented Mar 11, 2020

What do these changes do?

Extend the resource bodies, body-parts, and patches with properties making them easier to use, and add few handler kwargs for well-known and often-used fields (labels/annotations).

Description

Previously, it was quite common to use complicated Python structures to access the body/patch fields like this:

# OLD, as it was before:
@kopf.on.create(..., annotations={'some-marker': None})
def assign_ids(body, patch, **_):
    marker = body['metadata']['annotations']['some-marker']
    run_id = body.get('metadata', {}).get('labels', {}).get('run-id')
    field = body.get('spec', {}).get('field')
    patch.setdefault('status', {})['key'] = 'value'
    patch.setdefault('metadata', {}).setdefault('labels', {})['another-marker'] = 'value'

With this PR, well-known structures are exposed as body/patch properties, and are always behave as "live views" into their original dicts — i.e. every update on the dict is reflected in all views, all updates on the views is reflected in the original dict (similar to how KeysView, ValuesView, ItemsView work in Python itself).

# NEW, to be used:
@kopf.on.create(..., annotations={'some-marker': kopf.PRESENT})
def assign_ids(body, patch, labels, annotations, **_):
    marker = annotations['some-marker']
    run_id = labels.get('run-id')
    field = body.spec.get('field')
    patch.status['key'] = 'value'
    patch.meta.labels['another-marker'] = 'value'

Specifically:

  • JSON-decoded objects/dicts/fields are now explicitly typed as "raw" to make their origin clear. These are the lighweight structures coming directly from K8s API.
  • body kwarg is now always a "live view" to the original RawBody JSON-decoded dict, same as it already was for spec, meta, status kwargs before.
  • labels & annotations kwargs are added to all resource-related handlers.
  • body.spec|meta|status and body.meta.labels|annotations are the "live views" to a raw dict, persistent across multiple uses, and are exactly the same objects as provided in the kwargs (i.e. not re-created for every new handler).
  • patch follows the same .spec|meta|status semantics with "mutable live views".

In addition, the labels/annotations filters are changed to deprecate None as an "any value" marker, as it was confusing ("some value is None" does not sound natural). Instead, kopf.PRESENT or kopf.ABSENT tokens should be used for labels/annotations filters.

Purpose: All of the above adds some conventional features now for the event-driven handlers, but is critically important for the upcoming daemons/timers, where the bodies are updated over time, while the handlers that use these bodies, run in a sequential execution flow (unlike short event handlers).

For the end users, there are no big changes except for the new kwargs and properties available.

Slightly breaking: body is now not a dict, but a custom class, therefore it is not JSON-serialisable. Use dict(body) to make it JSON-serialisable. But there were no expected usage patterns to JSON-serialise the whole body, and here is a quick solution — so, should be fine.

Issues/PRs

Issues: #19

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

@zincr
Copy link

zincr bot commented Mar 11, 2020

🤖 zincr found 0 problems , 1 warning

ℹ️ Large Commits
✅ Approvals
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below


Large Commits

Checks all commits for large additions to a single file. Large commits should be reviewed more carefully for potential copyright and licensing issues

This file contains a substantial change, please review to determine if the change comes from an external source and if there are any copyright or licensing issues to be aware of

  • ℹ️ kopf/structs/bodies.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ tests/dicts/test_dictviews.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
     

@lgtm-com
Copy link

lgtm-com bot commented Mar 11, 2020

This pull request introduces 1 alert when merging 7ec7335 into 2c71453 - view on LGTM.com

new alerts:

  • 1 for Module imports itself

@nolar nolar added the enhancement New feature or request label Mar 11, 2020
docs/kwargs.rst Outdated
when a ``KeyError`` is raised.

``labels`` and ``annotations`` are equivalents of ``body['metadata']['labels']``
and ``body['metadata']['annotations']`` if they exits. If not, these two behave

Choose a reason for hiding this comment

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

typo: "exist"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

docs/walkthrough/updates.rst Show resolved Hide resolved


@kopf.on.create('zalando.org', 'v1', 'kopfexamples', labels={'somelabel': 'othervalue'})
def create_with_labels_not_satisfied(logger, **kwargs):
logger.info("Label not satisfied.")
def create_with_labels_mismatch(logger, **kwargs):

Choose a reason for hiding this comment

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

I don't understand this example. How is it different from create_with_labels_matching()? Between which values is the mismatch?
This function exists for the sake of assertions in the tests for the broader feature of filtering on labels, right? It is not meant to demonstrate any functionality different from create_with_labels_matching(). Consider adding a comment explaining this or (if it's not too much hassle), defining the function in the test file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right.

The examples are used as the functional tests. But we also have unit-test for labels/annotations matching & mismatching anyway. So, this precise level of testing is not needed. It can be reduced to pure demonstration of the markers and syntax only.

Removed.



@kopf.on.create('zalando.org', 'v1', 'kopfexamples', annotations={'someannotation': 'othervalue'})
def create_with_annotations_not_satisfied(logger, **kwargs):
logger.info("Annotation not satisfied.")
def create_with_annotations_mismatch(logger, **kwargs):

Choose a reason for hiding this comment

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

Same here: How is this different from create_with_annotations_matching()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also removed.

We already have unit-tests for the edge-case testing. The examples,
despite being used as functional tests, should only demo the features.
@lgtm-com
Copy link

lgtm-com bot commented Mar 11, 2020

This pull request introduces 1 alert when merging 39aaace into 2c71453 - view on LGTM.com

new alerts:

  • 1 for Module imports itself

@nolar nolar merged commit de23abe into zalando-incubator:master Mar 12, 2020
@nolar nolar deleted the dsl-of-bodies branch March 12, 2020 08:37
@nolar nolar added this to the 0.27 milestone May 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants