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

Add Upgrading to Nameko 3 section to docs #780

Draft
wants to merge 2 commits into
base: v3.0.0-rc
Choose a base branch
from

Conversation

iky
Copy link
Contributor

@iky iky commented Nov 3, 2023

No description provided.

Copy link

@pace-gene pace-gene left a comment

Choose a reason for hiding this comment

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

Saving this first half of the review and carrying on.

* Configuration loaded from a file or declared in the CLI is now available in a :ref:`global config object <nameko-config>`.
* Configuration may now be :ref:`declared on the CLI <cli-define-option>`.
* Configuration access via the service container is now deprecated.
* Builtin extensions and other facilities that read the configuration now access it via the global.

Choose a reason for hiding this comment

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

now access it via the global.

That isn't very grammatically correct or understandable. I suggest instead:

Builtin extensions and other facilities that access the configuration now access it via the nameko.config import rather than the deprecated service container.

CLI improvements
~~~~~~~~~~~~~~~~

* The CLI has been rebuilt using `click <http://click.pocoo.org/>`_ to be more robust, reliable and extensible..

Choose a reason for hiding this comment

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

rebuilt => rewritten

CLI improvements
~~~~~~~~~~~~~~~~

* The CLI has been rebuilt using `click <http://click.pocoo.org/>`_ to be more robust, reliable and extensible..

Choose a reason for hiding this comment

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

Two periods at the end of the sentence can become one.

~~~~~~~~~~~~~~~~

* The CLI has been rebuilt using `click <http://click.pocoo.org/>`_ to be more robust, reliable and extensible..
* The `backdoor` command has been removed; `run`` informs about how to connect to the backdoor port.

Choose a reason for hiding this comment

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

Two backquotes at the after run.

~~~~~~~~~~~~~~~~

* The CLI has been rebuilt using `click <http://click.pocoo.org/>`_ to be more robust, reliable and extensible..
* The `backdoor` command has been removed; `run`` informs about how to connect to the backdoor port.

Choose a reason for hiding this comment

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

I suggest the following:

The `backdoor` command has been removed; when executing the `run` command, the backdoor port is instead communicated on the standard output

**Required ⚠** — Read global `config` rather than any of the `x_config`` fixtures.
There is a breaking change to the way that the `rabbit_config`, `test_config` and `empty_config` fixtures work. They no longer return the config dictionary.

If you are using one of these fixtures, see the recommended pattern in the detailed breakdown of these configuration changes.

Choose a reason for hiding this comment

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

Could we link to the "detailed breakdown" here so users can easily see what they're expected to do?

....................

Required ⚠ — Don’t pass config to ServiceContainer or ServiceRunner
In Nameko 3, the ServiceContainer and ServiceRunner classes do not accept a config argument anymore. If you are programmatically creating a service runner, rather than using nameko run in the CLI, you must set up config in the global context before using them, for example with nameko.config.patch.

Choose a reason for hiding this comment

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

Format nameko.config.patch as code:

... example with `nameko.config.patch`.

....................

Required ⚠ — Don’t pass config to ServiceContainer or ServiceRunner
In Nameko 3, the ServiceContainer and ServiceRunner classes do not accept a config argument anymore. If you are programmatically creating a service runner, rather than using nameko run in the CLI, you must set up config in the global context before using them, for example with nameko.config.patch.

Choose a reason for hiding this comment

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

Please link to Config patching patching below so users can navigate quickly.

Required ⚠ — Don’t pass config to ServiceContainer or ServiceRunner
In Nameko 3, the ServiceContainer and ServiceRunner classes do not accept a config argument anymore. If you are programmatically creating a service runner, rather than using nameko run in the CLI, you must set up config in the global context before using them, for example with nameko.config.patch.

See the detailed breakdown of these changes for more information.

Choose a reason for hiding this comment

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

This had me looking for another document when I think you're mentioning the Config patching section below. Please reformulate to See below for the detailed breakdown [...] or See the *Config patching* section for [...].

Test Upgrades
.............

Although extensions will be backwards-compatible when used in services running Nameko 3, some changes may be required to their tests. These are the same changes that apply when :ref:`reading config inside tests for service code <using-config-in-tests>`.

Choose a reason for hiding this comment

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

It is Nameko that is backwards compatible with the extensions, not the reverse. Please reformulate as:

Although Nameko 3 is backwards-compatible with Nameko 2 extensions, some changes [...]

Other configuration related changes
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

* The `--broker`` CLI option is deprecated in favour of `--define` and `--config`. TODO: doesn’t actually raise a warning anymore

Choose a reason for hiding this comment

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

Is it worth opening an issue for this rather than have a TODO?

Config patching
~~~~~~~~~~~~~~~

The global `config` object has a `patch` helper object which can be used as a context manager or as a decorator. Here is an example of patching the config object in a pytest fixture:

Choose a reason for hiding this comment

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

As the config global object is basically just a dictionary, there is nothing to prevent the user from simply changing its values with a config["blah"] = 123. Please add an explanation as to why patch() is important. Namely because it only applies the patched changes to the current execution context and will revert to the initial values once out of the context.


Changes to the `container_factory` and `runner_factory` fixtures.

In Nameko 2, the `container_factory` and `runner_factory` fixtures accepted `config` as a required argument. In Nameko 3, it is optional. If passed, `nameko.config.patch` is used to wrap running container. The patch is applied before container start and returned it back to its original value when exiting the test:

Choose a reason for hiding this comment

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

Reformulate:

[...]. If config is passed, the factory will use config.patch() to emulate the old behaviour. This will cause the config values to be reverted to their original values once the test has exited.


def test_with_container_factory(container_factory):
container = container_factory(Service, config={"AMQP_URI": "..."})
container.start() # <- starts config patching scope, ending it when container

Choose a reason for hiding this comment

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

Aesthetics: comments are not lined vertically.


The optional argument provides backward compatibility so that tests for Nameko extensions can be compatible with both major versions.

When creating or upgrading existing services to run on Nameko 3, it is recommended to use the new config patch pattern instead:

Choose a reason for hiding this comment

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

Because using the alternative to config.patch() is deprecated, I would reformulate this in a more directive manner:

When creating or upgrading services to run on Nameko 3, manipulate config options through the new config.patch pattern like so:

* `rabbit_config`
* `web_config`

These fixtures no longer return the config dictionary. They simply add their configuration values to the existing `config` object.

Choose a reason for hiding this comment

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

Those functions make use of config.patch() as shims too, so the config change is transient and context-scoped (it's undone at the end of the current test). Please mention this.


These fixtures no longer return the config dictionary. They simply add their configuration values to the existing `config` object.

A recommended pattern for setting up a config object that draws from these fixtures is as follows:

Choose a reason for hiding this comment

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

Find below an example of how to set up the config values prior to running a test suite. The use of config.patch ensures those changes do not leak to other test suites.

@gawry
Copy link

gawry commented May 23, 2024

Hey, do you need any help with this?

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