Settings (official and documented) #336

nolar · 2020-03-26T16:55:02Z

What do these changes do?

Introduce settings: an official and documented way of configuring of all the little aspects of the framework behaviour on a per-operator basis.

Description

Previously, module-level hacks were used to modify global variables grouped into classes-as-namespaces (beside being undocumented):

import logging
import kopf

# DEPRECATED:
kopf.config.EventsConfig.events_loglevel = logging.WARNING
kopf.config.WatchersConfig.default_stream_timeout = 10 * 60
kopf.config.WorkersConfig.synchronous_tasks_threadpool_limit = 100

@kopf.on.create(...)
def create_fn(**_):
    pass

No more!

Settings are now a part of the framework, and should be defined in the startup handlers:

import logging
import kopf

@kopf.on.startup()
def configure(settings, **_):
    settings.posting.level = logging.WARNING
    settings.watching.stream_timeout = 10 * 60
    settings.execution.limit = 100

@kopf.on.create(...)
def create_fn(**_):
    pass

And there are many more settings (migrated from the existing config classes), and all the new settings will be added to this structure (e.g. a state persistence storage from #331).

import kopf

# NOT YET! SOON. See #331. 
@kopf.on.startup()
def configure(config: kopf.OperatorConfig, **_):
    config.persistence.storage = kopf.AnnotationsStateStorage(prefix='my-opr.example.com')
    # OR:
    config.persistence.storage = kopf.StatusStateStorage(name='my-opr')

Backward-compatibility

The legacy way via module-level config-classes is supported — they are used as the defaults for the settings. BUT: runtime changes of the config-classes are ignored, only the runtime changes of the settings are obeyed.

A special case with the executor of synchronous handlers is supported, and calls to set_synchronous_tasks_threadpool_limit() are forwarded to the settings executor — until config-classes are removed with the next major release (1.0).

Beside exposing settings to the users officially, this change also adds a dependency injection mechanics which can be used in the tests instead of mocks — thus making the tests cleaner.

Issues/PRs

Issues: #23

Related: #322 #192 #109

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 · 2020-03-26T16:55:14Z

🤖 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

ℹ️ docs/configuration.rst had +100 lines of code changed in a single commit
Please review this file to determine the source of this change
ℹ️ kopf/structs/configuration.py had +100 lines of code changed in a single commit
Please review this file to determine the source of this change

zincr · 2020-03-26T16:55:14Z

🤖 zincr found 1 problem , 1 warning

❌ Approvals
ℹ️ Large Commits
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below

Approvals

All proposed changes must be reviewed by project maintainers before they can be merged

Not enough people have approved this pull request - please ensure that 1 additional user, who have not contributed to this pull request approve the changes.

✅ Approved by PR author @nolar
❌ 1 additional approval needed

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

ℹ️ docs/configuration.rst had +100 lines of code changed in a single commit
Please review this file to determine the source of this change
ℹ️ kopf/structs/configuration.py had +100 lines of code changed in a single commit
Please review this file to determine the source of this change

lgtm-com · 2020-03-26T17:03:31Z

This pull request introduces 2 alerts when merging d94070f into aa80d5a - view on LGTM.com

new alerts:

2 for Module-level cyclic import

mnarodovitch

Great.

haikoschol

~~Just some wording nits in the docs. Feel free to ignore if you'd rather merge now.~~ nvm :D

haikoschol · 2020-03-27T14:26:58Z

docs/configuration.rst

+with a watching request will exist before terminating from the **client** side.
+The default is forever (``None``).
+
+``settings.watching.stream_timeout`` (seconds) is how long the session


I would have never guessed that from the names. How about session_client_timeout and session_server_timeout?

haikoschol · 2020-03-27T14:27:47Z

docs/configuration.rst

+Configuration
+=============
+
+There are tools to configure some of kopf functionality, like asynchronous


"Tools" makes me think of things like cmdline programs. How about "It is possible to configure..."?

haikoschol · 2020-03-27T14:29:06Z

docs/configuration.rst

+Startup configuration
+=====================
+
+Every operator has its settings (even if there are more than one operator


grammar nit: I'd word it as "... even if there is more than one operator in the same process ..."

haikoschol · 2020-03-27T16:14:41Z

docs/configuration.rst

+====================
+
+``settings.execution`` allows to set a number of synchronous workers used
+and redefined the asyncio executor:


nit: "redefines", otherwise the sentence might first be parsed as "number of synchronous workers used and redefined".

Or even better, find a different word for "redefines" since that suggests the implementation of the executor is changed when I think it just gets reconfigured or maybe another object is instantiated with different parameters?

I would also change "a number" to "the number". With the former the sentence can be misunderstood as "some of the synchronous workers are somehow changed". (This is really grammar nitpicking. I expect most people who read this will know what it means as is.)

haikoschol · 2020-03-27T16:18:50Z

docs/kwargs.rst

+which defines the operator's behaviour. See also: `kopf.OperatorSettings`.
+
+It can be modified if needed (usually in the startup handlers). Every operator
+(if there are more than one in the same process) has its own config.


nit: if there is more than one

nolar · 2020-03-27T21:49:07Z

@haikoschol Thank you. I've implemented these recommendation with few little additions in #337.

nolar added 10 commits March 26, 2020 16:35

Pass a placeholder settings container down the stack where needed

84b79ac

Extend KopfRunner testing toolkit with user-provided settings & registry

49fef44

Switch logging and posting from configs to settings

1940afa

Switch K8s event-watching from configs to settings

773b4ff

Switch demultiplexer's event-queue handling and batching to use settings

4782497

Switch thread-pool executor for sync handlers from configs to settings

80e8a28

Put the deprecation markers for the old configs

6b0e321

Adjust tests to use new per-test settings instead of configs & mocks

399ae9b

Add a setting to disable log message posting without level trickery

846c1cd

Document settings in a new way via settings (instead of old configs)

d94070f

nolar added the enhancement New feature or request label Mar 26, 2020

nolar requested a review from haikoschol March 26, 2020 16:55

Resolve cyclic module imports as detected by LGTM.com

a1cf3c2

nolar force-pushed the configs branch from c5f3997 to a1cf3c2 Compare March 26, 2020 17:42

nolar requested a review from mnarodovitch March 27, 2020 12:45

mnarodovitch approved these changes Mar 27, 2020

View reviewed changes

nolar merged commit 37edf2d into zalando-incubator:master Mar 27, 2020

nolar deleted the configs branch March 27, 2020 17:48

haikoschol approved these changes Mar 27, 2020

View reviewed changes

nolar mentioned this pull request Mar 27, 2020

Better naming for timeouts + connection timeout #337

Merged

5 tasks

nolar added this to the 0.27 milestone May 11, 2020

kopf-archiver bot mentioned this pull request Aug 19, 2020

[PR] Settings (official and documented) nolar/kopf#336

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Settings (official and documented) #336

Settings (official and documented) #336

nolar commented Mar 26, 2020 •

edited

Loading

zincr bot commented Mar 26, 2020 •

edited

Loading

zincr bot commented Mar 26, 2020

lgtm-com bot commented Mar 26, 2020

mnarodovitch left a comment

haikoschol left a comment •

edited

Loading

haikoschol Mar 27, 2020

haikoschol Mar 27, 2020

haikoschol Mar 27, 2020

haikoschol Mar 27, 2020

haikoschol Mar 27, 2020

nolar commented Mar 27, 2020

Settings (official and documented) #336

Settings (official and documented) #336

Conversation

nolar commented Mar 26, 2020 • edited Loading

What do these changes do?

Description

Backward-compatibility

Issues/PRs

Type of changes

Checklist

zincr bot commented Mar 26, 2020 • edited Loading

🤖 zincr found 0 problems , 1 warning

Large Commits

zincr bot commented Mar 26, 2020

🤖 zincr found 1 problem , 1 warning

Approvals

Large Commits

lgtm-com bot commented Mar 26, 2020

mnarodovitch left a comment

Choose a reason for hiding this comment

haikoschol left a comment • edited Loading

Choose a reason for hiding this comment

haikoschol Mar 27, 2020

Choose a reason for hiding this comment

haikoschol Mar 27, 2020

Choose a reason for hiding this comment

haikoschol Mar 27, 2020

Choose a reason for hiding this comment

haikoschol Mar 27, 2020

Choose a reason for hiding this comment

haikoschol Mar 27, 2020

Choose a reason for hiding this comment

nolar commented Mar 27, 2020

nolar commented Mar 26, 2020 •

edited

Loading

zincr bot commented Mar 26, 2020 •

edited

Loading

haikoschol left a comment •

edited

Loading