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

Global nameko config #520

Merged
merged 56 commits into from
Jan 4, 2019
Merged

Global nameko config #520

merged 56 commits into from
Jan 4, 2019

Conversation

iky
Copy link
Contributor

@iky iky commented Mar 8, 2018

A draft of config made accessible outside container and workers - also in the service
definition, e.g.:

from nameko.messaging import Consumer
from nameko import config

class Service:

    @consume(
        queue=Queue(
            exchange=config.MY_EXCHANGE,
            routing_key=config.MY_ROUTING_KEY,
            name=config.MY_QUEUE_NAME
        ),
        prefetch_count=config.MY_CONSUMER_PREFETCH_COUNT
    )
    def consume(self, payload):
        pass

It will also help with setting optionals by simply passing them to the constructor:

from nameko import config
from nameko_sqlalchemy import Database

class Service:
    db = Database(engine_options=config.DB_ENGINE_OPTIONS)

Extensions could load from config only entries necessary for them to run (for example in the Database case it would be the DB connection). Anything optional can be passed on instantiation using settings loaded from config. Also this way users are free to structure configs their way.

iky added 2 commits March 8, 2018 17:21
Config made accessible outside container and workers - mainly in the service
definition, e.g.:

```python

from nameko.messaging import Consumer
from nameko import config

class Service:

    @consume(
        queue=Queue(
            exchange=config.MY_EXCHANGE,
            routing_key=config.MY_ROUTING_KEY,
            name=config.MY_QUEUE_NAME
        ),
        prefetch_count=config.MY_CONSUMER_PREFETCH_COUNT
    )
    def consume(self, payload):
        pass

```
Example::
  $ nameko run service \
  --define AMQP_URI=pyamqp://guest:guest@localhost/yo \
  --define YO="{'yo': True}"
@iky
Copy link
Contributor Author

iky commented Mar 9, 2018

Added the possibility to define config entries from console (overrides possible config file entries):

$ nameko run service \
--define AMQP_URI=pyamqp://someuser:*****@somehost/ \
--define max_workers=1000 \
--define SLACK="{'TOKEN': '*****'}"

@iky
Copy link
Contributor Author

iky commented Mar 23, 2018

Remaining Changes

In order to fully move to the global config, some additional things need to be solved, such as backward compatibility so the change will not brake 3rd-party extensions. Also pytest fixtures should keep existing config related features.

Backward compatible Config on Container

Built-in extensions should be changed to use nameko.config. 3rd party extensions should then catch up and migrate too:

from nameko import config

class Database(DependencyProvider):
    def setup(self):
        db_uris = config[DB_URIS_KEY]
        # ...

For making the changes backward compatible, the container config if accessed should raise a deprecation warning saying that the nameko.config should be used instead, but still returning the expected config.

The Container.config property after raising the deprecation warning should return nameko.config keeping the 3rd-party extensions still working:

class Database(DependencyProvider):
    def setup(self):
        db_uris = self.container.config[DB_URIS_KEY]  # <- should still work
        # ...

Only issuing a warning:

.../nameko/containers.py:173: DeprecationWarning: Use ``nameko.config`` instead.
  warnings.warn("Use ``nameko.config`` instead.", DeprecationWarning)

Config setup helpers

Mainly for tests, but also for custom runners requiring in-program config setup, it would be good to have a helper function for setting up config in a context...

from nameko import config_setup

with config_setup(my_config):
    ServiceContainer(Service)

...rolling back config changes on the context exit. The context manager allows for sync use of multiple different configs per program which I think has practical use only in tests.

The config_setup should be available as a simple function too - useful for bootstraps and scripts. The CLI commands of Nameko maybe should use it too when setting up config from file and from cli options.

Container and Runner

Other places documented as part of Nameko API and using config are service and container runners.

ServiceContainer(Service, config={})
ServiceRunner(config={})

I would propose removing the config passing completely as a breaking change:

ServiceContainer(Service)
ServiceRunner()

If a specific config should be used, it can be achieved by using config_setup context manager described above.

Standalone

Same approach as with the other runners, but reusing the existing context manager in case of standalone RPC proxy. By making the config optional argument of ClusterRpcProxy, one can still do

from nameko.standalone.rpc import ClusterRpcProxy

config = {
    'AMQP_URI': AMQP_URI  # e.g. "pyamqp://guest:guest@localhost"
}

with ClusterRpcProxy(config) as cluster_rpc:
    cluster_rpc.service_x.remote_method("hellø")  # "hellø-x-y"

If a config is passed, the proxy context is also wrapped in config_setup, otherwise nameko global config is used as it is.

In case of the (so far undocumented) standalone event publisher, the dispatcher can be also made context manager with optional config:

from nameko.standalone.events import event_dispatcher

config = {
    'AMQP_URI': AMQP_URI  # e.g. "pyamqp://guest:guest@localhost"
}

with event_dispatcher(config) as dispatcher:
    dispatcher.dispatch('some_service', 'some_event', {'some': 'payload'})

or by using config_setup as context manager (probably only if two clusters are involved, or some other complex case):

from nameko import config_setup
from nameko.standalone.events import event_dispatcher

with config_setup(config_cluster_foo):
    dispatcher = event_dispatcher()
    dispatcher.dispatch('some_service', 'some_event', {'some': 'payload'})

with config_setup(config_cluster_bar):
    dispatcher = event_dispatcher()
    dispatcher.dispatch('some_other_service', 'some_event', {'some': 'payload'})

or simply as as a function (in a script or when bootstrapping something more complex than a simple script):

from nameko import config_setup
from nameko.standalone.events import event_dispatcher

config = {
    'AMQP_URI': AMQP_URI  # e.g. "pyamqp://guest:guest@localhost"
}

config_setup(config):
dispatcher = event_dispatcher()
dispatcher.dispatch('some_service', 'some_event', {'some': 'payload'})

Maybe all three should work. The last two should definitely work.

Testing

Config Fixtures

Existing rabbit_config, web_config and maybe empty_config will be kept but made all pytest.yield_fixture generators using the config_setup helper to "reset" the config after each use. Rabbit config already is a generator - it's yield will only get and extra wrap with config_setup.

Usage of existing config fixtures remain the same except they will not be passed anymore to container or runner factories:

def test_service_x_y_integration(runner_factory, rabbit_config):
    runner = runner_factory(ServiceX, ServiceY)
    runner.start()
    # ...

Not sure what is the most correct pytest way if a fixture is not used inside the test function, maybe the use_fixture decorator is then better:

@pytest.mark.usefixtures('rabbit_config')
def test_service_x_y_integration(runner_factory):
    runner = runner_factory(ServiceX, ServiceY)
    runner.start()
    # ...

Users can define test config for their services reusing existing nameko config fixtures::

from nameko import config_setup

@pytest.fixture(autouse=True)
def config(web_config, rabbit_config):
    config = {
        # some testing config, defined in place or loaded from file ...
    }
    config.update(web_config)
    config.update(rabbit_config)
    with config_update(config):
        yield

Factory fixtures

Both container_factory and runner_factory fixtures take config as required argument. Passing the config can stay making it only optional which if passed would apply coinfig_setup context manager. Other option would be to remove it as a breaking change similarly as with ServiceContainer and ServiceRunner.

@pytest.mark.usefixtures('rabbit_config')
def test_event_interface(container_factory):
    container = container_factory(ServiceB)
    container.start()
    # ...

Deprecating Config Dependency Provider

Global config would make the Config DD obsolete and deprecated. The get_dependency should raise a deprecation warning.

Copy link
Member

@mattbennett mattbennett left a comment

Choose a reason for hiding this comment

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

This is an excellent suggestion and a diligent summary of all the required changes to make use of the global configuration. I am in favour of all of your suggestions.

A global config object has been requested before and I pushed back based on the importance of dependency injection in Nameko. But configuration is clearly different and this change will make it much easier to write extensions, runners and other enhancements without compromising the general principle of injecting dependencies.

Re: mutability of the config object, I don't really mind. If there is some out of the box pattern that makes immutability easy, we could use it. If it's not easy I'd prefer to leave things permissive and allow people to abuse it at their own risk.

AMQP_URI_CONFIG_KEY: args.broker
}
if AMQP_URI_CONFIG_KEY not in config:
config.update({AMQP_URI_CONFIG_KEY: args.broker})
Copy link
Member

Choose a reason for hiding this comment

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

Let's deprecate the --broker flag arg with this change. You'll be able to use --define AMQP_URI=whatever instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 deprecated - raising a warning, but still setting the broker uri.

broker_from = ""
else:
broker_from = " (from --config)"
Copy link
Member

Choose a reason for hiding this comment

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

This could actually be from --define now. I'm not sure this message is helpful enough to keep.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I removed it from the banner.


def _command(*argv):
with patch.object(sys, 'argv', argv):
main()
Copy link
Member

Choose a reason for hiding this comment

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

This is very neat!

@mattbennett
Copy link
Member

@iky can you give a quick update on the state of this branch?

@iky
Copy link
Contributor Author

iky commented Sep 27, 2018

@mattbennett I should get back to it very very soon!

@iky iky changed the title [WIP] Global nameko config Global nameko config Dec 13, 2018
@iky
Copy link
Contributor Author

iky commented Dec 14, 2018

This is now ready to be reviewed.

Besides adding nameko.config and --define, this PR includes also the following changes:

  • Container.config is now deprecated in favour of nameko.config and no built-in Nameko component uses it any more. It still returns, so existing community extensions would work. A deprecation warning is raised if the container config is accessed.
  • No Nameko component gets config passed now - nameko.config is used instead. Therefore there are braking changes - non of the following takes config as an argument anymore:
    • ServiceContainer and ServiceRunner.
    • Standalone ClusterRpcClient, ServiceRpcClient and event_dispatcher.
    • Testing fixtures container_factory and runner_factory.
  • nameko.config_update and nameko.config_setup helper functions and context managers added. These are used only in tests (config update example, another update example, config setup example). There is a question, should these helpers then be moved to testing module?
  • There is a partial change to the use of empty_config, rabbit_config and web_config which now do NOT return the config dictionary any more, instead they use nameko.config_update to update nameko.config just for the context of the tests these fixture are used for - this is also a braking change. Existing tests changed accordingly.
  • Built-in Config dependency provider deprecated.
  • --broker CLI option is deprecated in favour of --define and --config. Still works (for now) but raises a deprecation warning.
  • All messaging dependency providers (publisher, RPC publishers and events dispatchers) now take uri as an argument setting up rabbit URI (just for the dependency, defaults to AMQP_URI config setting). Here's ServiceRpc example.

Copy link
Member

@mattbennett mattbennett left a comment

Choose a reason for hiding this comment

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

This looks great! Just a few small comments, then we should get this merged in 👍

nameko/standalone/rpc.py Outdated Show resolved Hide resolved
nameko/testing/pytest.py Show resolved Hide resolved
nameko/testing/pytest.py Show resolved Hide resolved
nameko/testing/pytest.py Show resolved Hide resolved
test/cli/test_main.py Show resolved Hide resolved
test/cli/test_shell.py Show resolved Hide resolved
test/test_config.py Outdated Show resolved Hide resolved
test/test_events.py Outdated Show resolved Hide resolved
test/test_rpc.py Show resolved Hide resolved
@mattbennett
Copy link
Member

nameko.config_update and nameko.config_setup helper functions and context managers added.
There is a question, should these helpers then be moved to testing module?

I don't think they do any harm in the main namespace. There may well be non-testing usecases for them, so perhaps they should stay where they are.

@mattbennett
Copy link
Member

What do you think about apply_config (or set_config) and update_config instead of config_setup and config_update? Verb-first feels a little more natural to me.

@iky
Copy link
Contributor Author

iky commented Jan 4, 2019

What do you think about apply_config (or set_config) and update_config instead of config_setup and config_update? Verb-first feels a little more natural to me.

Yep, agree, will change it ...

@iky
Copy link
Contributor Author

iky commented Jan 4, 2019

What do you think about apply_config (or set_config) and update_config instead of config_setup and config_update? Verb-first feels a little more natural to me.

Yep, agree, will change it ...

Done

Copy link
Member

@mattbennett mattbennett left a comment

Choose a reason for hiding this comment

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

👍 Brilliant. Thank you @iky!

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.

None yet

2 participants