-
Notifications
You must be signed in to change notification settings - Fork 3
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
[WIP] Atualização geral do projeto #22
base: production
Are you sure you want to change the base?
Conversation
Adição de suporte a swagger
Adição de response intecerptor para transformar a saída no padrão RESTful (em progresso) além de adicionar custom headers: custom-arch-version: v1 custom-service-version: 1.0.0
docker - Melhorias para a instalação e execução do projeto; docs - Inclusão de exemplos de imagens com informações relevantes do projeto; samples - Pasta para exemplos rápidos do projeto; CHANGELOG.md - Arquivo de alterações do projeto; Atualização do Nest para a versão 8
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
@@ -0,0 +1,21 @@ | |||
MIT License | |||
|
|||
Copyright (c) 2021 Anderson Contreira |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we replace "Anderson Contreira" with "MadeiraMadeira"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nossa, é verdade kkk, copie e colei e nem vi, Valeu!
|
||
|
||
## Service Architecture | ||
This service make part of the project <project name>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a typo in here: this service is part* not "make part".
![Swagger OpenApi](docs/service-arch.png) | ||
> Obs: image to describe the service architecture or project architecture. | ||
|
||
## General Kong Routes Architecture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What this section actually is?
## Stack | ||
|
||
- [NestJS](https://github.com/nestjs/nest): a progressive Node.js + TypeScript framework based on dependency injection; | ||
- [gRPC](https://grpc.io): a Remote Procedure Call framework that provides high performance communication between microservices. | ||
- [Swagger + OpenAPI](https://swagger.io/specification/): an OpenAPI definition can then be used by documentation generation tools to display the API, code generation tools to generate servers and clients in various programming languages, testing tools, and many other use cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This text contains unusual English expressions. I would replace it with this:
"an OpenAPI definition can be used by document generation tools -- such as Swagger -- to automatically generate API docs. It can also be used by code generation tools to create servers, clients, tests, and so on."
- [Swagger + OpenAPI](https://swagger.io/specification/): an OpenAPI definition can then be used by documentation generation tools to display the API, code generation tools to generate servers and clients in various programming languages, testing tools, and many other use cases. | ||
|
||
## Swagger Docs | ||
The current documentation of the project can be found on url `/docs`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something like this would sound more natural:
"The API documentation can be found at this URL: {host}/docs
."
``` | ||
|
||
## Samples | ||
See the project samples in this folder [here](samples). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To sound more natural:
"Check out the project samples at the samples directory"
to get some information about ReportOptions look [here](https://use-form.netlify.app/interfaces/_node_modules__types_istanbul_reports_index_d_.reportoptions.html). | ||
|
||
## Running test | ||
To run the unit tests of the project you can execute the follow command: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
following*
$ mv nest-service-template-production {your_project_name} | ||
$ git init | ||
$ git remote add origin https://github.com/{user}/{repo}.git | ||
mv skeleton-nest-microservice-api {your_project_name} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would call the project nestjs-service-template
. It is simple and pretty accurate.
@@ -0,0 +1,9 @@ | |||
import { APP_NAME, APP_VERSION } from '../main'; | |||
import { Controller, Get, Header } from '@nestjs/common'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
be aware of unused imports like Header
'🍻️ Core APIs Nest Service Template gRPC layer listening on port ' + | ||
grpcPort, | ||
); | ||
logger.log(`🍻️ ${APP_NAME} API layer listening on port ${restPort}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would return this to "REST layer" as it was before. Technically, the gRPC layer has an API too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Please notice:
- I think the best name for this project is
nestjs-service-template
because its simple; - the default PR and commit message language is English;
- we follow commit message standards, as you can see in the commit history, that could also be used to automatically generate changelogs. You can ask someone from Core APIs to get to know our commit message standards;
- this PR is too large, has lots of different contexts, and could be separated into small PRs to facilitate the code review;
- I wasn't able to hit
GET /
,GET /alive
norGET /health
:
ERROR [ExceptionsHandler] rxjs_1.lastValueFrom is not a function
TypeError: rxjs_1.lastValueFrom is not a function
Note: I've run the application with ./bin/runenv.sh --build
.
- `GET /` | ||
- `GET /alive` | ||
- `GET /docs` | ||
- `GET /v1/product` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does GET /v1/product
should actually be here?
@Injectable() | ||
export class RequestInterceptor implements NestInterceptor { | ||
intercept(context: ExecutionContext, next: CallHandler): Observable<any> { | ||
console.log('Before...'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should avoid use console.log
networks: | ||
- default | ||
# localstack: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COPY package-lock.json ./ | ||
|
||
# python and g++ are mandatory due to Shards-Dashboard | ||
RUN apk --no-cache --virtual build-dependencies add \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why added build-dependencies and heavy stuff to this simple template ?
return next | ||
.handle() | ||
.pipe( | ||
tap(() => console.log(`After... ${Date.now() - now}ms`)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too
|
||
@Controller('health') | ||
export class HealthController { | ||
constructor(private healthService: HealthService) {} | ||
|
||
@Get() | ||
@ApiResponse({ | ||
status: 201, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use 201 and not 200 for health check ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The right code is 200, was a typing mistake here
@@ -0,0 +1 @@ | |||
docker-compose up $1 $2 $3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why wrapping docker-compose inside a .sh script? Also, is runenv.sh
a descriptive name for what it actually does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use kebab-case for file names, as the other project files.
Thank you guys, for the commentaries here, maybe tomorrow I gonna adjust some points and clarify others, I didn't finish yet this package of improvements, the missed routes I will implement to let a simple RESTful example on this project based on the diagram (store -> products/categories). |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Proposed changes
Describe your changes briefly and objectively.
Checklist