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

feat: adding artifacts for shopping cart #4

Merged
merged 4 commits into from
Aug 30, 2018
Merged

feat: adding artifacts for shopping cart #4

merged 4 commits into from
Aug 30, 2018

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Aug 16, 2018

Add model/datasource/repository for redis backed shopping carts.

See loopbackio/loopback-next#1481

Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

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

Tests + A script to setup docker?

@raymondfeng raymondfeng force-pushed the kv-redis branch 2 times, most recently from f09a45b to f1fe284 Compare August 21, 2018 17:14
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Good start.

},
},
});
await container.start();
Copy link
Member

Choose a reason for hiding this comment

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

How long does it take to start a Redis container?

I am concerned about the overhead this may be adding to npm t. When developing locally, we don't want to (re)start the Redis instance every time we want to re-run our test suite.

Instead, we should start any database instances beforehand. When running on CI, the CI build script should start the instances before running the tests - Travis CI provides hooks for that.

See also #5. Since Travis CI already supports Redis, I think we should use the same approach from #5 and use Travis-CI provided instance of Redist instead of using Docker.

Related best practice: Clean the database before each test.

userId: 'u01',
items: [
{
productId: 'p1',
Copy link
Member

Choose a reason for hiding this comment

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

Please follow our best practices, in particular Use data builders.

Signed-off-by: Raymond Feng <enjoyjava@gmail.com>
@raymondfeng raymondfeng force-pushed the kv-redis branch 2 times, most recently from 7f29c8f to bf2af9e Compare August 28, 2018 17:12
@raymondfeng
Copy link
Contributor Author

@bajtos @virkt25 PTAL

@raymondfeng raymondfeng force-pushed the kv-redis branch 2 times, most recently from e8591c5 to 18ae2b6 Compare August 28, 2018 18:09
Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

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

Will a controller be added in a followup task? Should we use a @hasMany relation between a ShoppingCart and ShoppingCartItem?

ShoppingCart
> {
constructor() {
super(ShoppingCart, new RedisDataSource());
Copy link
Contributor

Choose a reason for hiding this comment

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

The DataSource should be injected.

Signed-off-by: Raymond Feng <enjoyjava@gmail.com>
Signed-off-by: Raymond Feng <enjoyjava@gmail.com>
@raymondfeng
Copy link
Contributor Author

Will a controller be added in a followup task? Should we use a @hasmany relation between a ShoppingCart and ShoppingCartItem?

Let's try to add the basic support 1st.

bin/start-dbs.sh Outdated
#!/bin/bash
if [ -z "$CI" ]; then
MONGO_CONTAINER_NAME="mongodb_c"
docker rm -f $MONGO_CONTAINER_NAME
Copy link
Contributor

Choose a reason for hiding this comment

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

is it a dup to remove it again in the start file? I notice that the same cmd is called in stop-dbs.sh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, I added stop-dbs.sh so that I can shut down these containers manually for local tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just to be sure that such containers are removed. We can call ./stop-dbs.sh instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not really, I added stop-dbs.sh so that I can shut down these containers manually for local tests.

Oh I see 👌

@@ -0,0 +1,7 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

should travis call this file after test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Travis recycles the containers. We don't have to worry about the stop. BTW, our Travis Mac build uses brew.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, Travis pretty much kills the instance that runs the tests once the tests are done ... and we aren't starting a docker containre on Travis but a service.

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

Mostly LGTM 👍
Just a concern for defining a class without @model, I think we don't generate OpenAPI schema for it, which means you could not refer to the spec by #/components/schemas/ShoppingCartItem.

/**
* Item in a shopping cart
*/
export class ShoppingCartItem {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am afraid we don't generate OpenAPI schema for a class without decorating it with @model and @property.

Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

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

Minor nitpick. Looks ready to land otherwise.

package.json Outdated
@@ -20,7 +20,7 @@
"clean": "lb-clean dist*",
"commit": "git-cz",
"commitmsg": "commitlint -E GIT_PARAMS",
"docker": "./bin/setup.sh",
"docker": "./bin/start-dbs.sh",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a docker:stop which calls stop-dbs.sh.

@@ -0,0 +1,7 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

No, Travis pretty much kills the instance that runs the tests once the tests are done ... and we aren't starting a docker containre on Travis but a service.

Signed-off-by: Raymond Feng <enjoyjava@gmail.com>
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

LGTM.

@raymondfeng raymondfeng merged commit 9320192 into master Aug 30, 2018
@raymondfeng raymondfeng deleted the kv-redis branch August 30, 2018 15:48
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.

4 participants