Orders #1
Orders #1
Conversation
|
|
||
| def delete(self): | ||
| if self.deleted_at is None: | ||
| self.deleted_at = datetime.datetime.utcnow() |
There was a problem hiding this comment.
I wonder if using our Base is too much for this example. It's a nice pattern but may be confusing since it's non-essential.
Maybe we can keep it but remove the "soft-delete"?
There was a problem hiding this comment.
I agree. We know soft-delete is potentially an anti-pattern. I'll remove it.
|
Looking for feedback. @mattbennett @davidszotten @timbu @iky |
| name = 'orders' | ||
|
|
||
| db = DatabaseSession(DeclarativeBase) | ||
| order_schema = OrderSchema() |
There was a problem hiding this comment.
It's probably cleaner to use OrderSchema directly, rather than attaching it as a class attribute.
|
|
||
| @pytest.fixture | ||
| def container(container_factory, config): | ||
| ServiceMeta = namedtuple( |
There was a problem hiding this comment.
There's a newer version of this pattern that allows replacement values to be specified.
| @@ -0,0 +1,74 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
Might be worth a module-level docstring here to define the scope of an "interface" test for a nameko service
There was a problem hiding this comment.
Or possibly interface/conftest.py is a better place for it
|
|
||
| @pytest.fixture(scope='session') | ||
| def db_url(): | ||
| """Overriding db_url fixture from nameko_sqlalchemy""" |
There was a problem hiding this comment.
We probably also need to explain somewhere where the db_session etc fixtures come from, and why these overrides are necessary.
|
|
||
| # Run Migrations | ||
|
|
||
| alembic upgrade head |
There was a problem hiding this comment.
Probably worth nothing that you cannot do backwards migrations with this docker container, and that this is just to get you started.
Alternatively we could remove alembic completely from the example app.
There was a problem hiding this comment.
I would definitely like to keep alembic in the example app. Migrations is a very basic part of any db dependent service. I think we should mention how to support backward migrations in readme for this example alongside of other things that you can find here but are only done for example sake and are not recommended like having multiple services in one repo.
|
Comments addressed |
| @@ -23,4 +23,4 @@ alembic upgrade head | |||
|
|
|||
| # Run Service | |||
|
|
|||
| nameko run --config config.yml orders.service --backdoor 3000 | |||
There was a problem hiding this comment.
Might be nice to show the backdoor here too
| RABBIT_PASSWORD: "guest" | ||
| RABBIT_USER: "guest" | ||
| RABBIT_HOST: "rabbit" | ||
| RABBIT_PORT: "15672" |
There was a problem hiding this comment.
Actually there is a problem here: RABBIT_PORT is used to construct both the AMQP_URI in config.yaml and in run.sh::is_ready(). You need 5672 for one and 15672 for the other.
| version: "2" | ||
| services: | ||
| rabbit: | ||
| container_name: rabbitmq |
There was a problem hiding this comment.
These generic container names (rabbitmq, postgres_db, orders) could clash with other existing containers running in the docker machine. Since these are just used for the nameko example, we could prepend some kind of prefix to them to make more explicit what they are used for (like nameko-example-...). What do you think?
There was a problem hiding this comment.
I was thinking the same. I'll add more explicit prefixes.
| 'alembic==0.8.7', | ||
| 'marshmallow==2.9.1', | ||
| 'psycopg2==2.6.2', | ||
| 'alembic==0.8.7' |
|
|
||
|
|
||
| # @pytest.mark.usefixtures('orders_service') | ||
| @pytest.yield_fixture |
There was a problem hiding this comment.
yield_fixture is going to be deprecated soon. We could upgrade pytest to the latest version in the setup.py and just use @pytest.fixture here.
| return create_service_meta('event_dispatcher') | ||
|
|
||
|
|
||
| # @pytest.mark.usefixtures('orders_service') |
There was a problem hiding this comment.
Is this comment here intentionally?
…ld pytest fixture.
|
@juliotrigo your comments addressed. |
* k8s Deployment files created * Ingress added. Fixes to products config file. * Definitions converted to charts * Tutorial added * Readme updates * Update to Readme * Nameko loves Kubernetes * Update to Readme * Update to readme. * Update to Readme * Readme updates * Bump nameko version
Orders service.
nameko/nameko-example-orders:latest