Ankith onboard #156
Ankith onboard #156
Conversation
…juno into ankith-onboard Merge corepack fix
| We utilize Nest's [OpenAPI](https://docs.nestjs.com/openapi/introduction) support to autogenerate web documentation for all Juno HTTP endpoints. | ||
|
|
||
| When running Juno locally, the documentation can be found under `localhost:<api-gateway port from docker>/docs`. If using the deployment version of Juno, engineering leadership will provide you with a valid documentation page. Note that the api-gateway port is *not* the isolated docker port (e.g. port 3000) but the exposed host port. | ||
| When running Juno locally, the documentation can be found under `localhost:<api-gateway port from docker>/docs`. If using the deployment version of Juno, engineering leadership will provide you with a valid documentation page. Note that the api-gateway port is _not_ the isolated docker port (e.g. port 3000) but the exposed host port. |
There was a problem hiding this comment.
Be slightly careful about linters automatically changing files--this could lead to merge conflicts later on
| await app.init(); | ||
| }); | ||
|
|
||
| const ADMIN_EMAIL = 'test-superadmin@test.com'; |
There was a problem hiding this comment.
Note for future reference: the test admin credentials should be consolidated in one place (at the moment we have ~10 declarations of this, making it tedious if the seed were to ever change)
| @@ -25,7 +25,7 @@ | |||
| "@grpc/grpc-js": "^1.9.7", | |||
| "@nestjs/common": "^10.0.0", | |||
| "@nestjs/config": "^3.1.1", | |||
| "@nestjs/core": "^10.0.0", | |||
| "@nestjs/core": "11.0.2", | |||
There was a problem hiding this comment.
Try to avoid any sort of major version upgrade unless it was intended for the ticket
| @@ -52,6 +52,7 @@ Juno packages are built with [Nest.js](https://docs.nestjs.com/) and follow a st | |||
| ``` | |||
|
|
|||
| ## Database Overview | |||
|
|
|||
There was a problem hiding this comment.
Mentioned above, but be careful about accidentally modifying files (check your staged changes before committing)
| @@ -134,3 +134,9 @@ model FileServiceFile { | |||
|
|
|||
| @@id([path, bucketName, configId]) | |||
| } | |||
|
|
|||
| //Counter | |||
There was a problem hiding this comment.
nit: no need for extra comment, model name is self explanatory
| //Get counter will create a counter if not found. | ||
| const counter = await this.prisma.counter.findUnique({ where: { id: id } }); | ||
|
|
||
| if (counter == null) { |
There was a problem hiding this comment.
Be careful about adding any unintended side effects to a method. In this case, automatically creating a counter if it doesn't exist is a mutation side effect. It should be up to the developer who called this method to determine the next course of action (perhaps they only wanted to check if it exists and display a message to the user). Another example to illustrate this: if I called getCar() to check what car I might have and it decided to purchase a bugatti, my bank account would be quite unhappy :)
|
|
||
| @Get(':id') | ||
| async getCounterById(@Param('id') id: string): Promise<CounterResponse> { | ||
| //Super jank, { id:{id: id} } because returnin |
There was a problem hiding this comment.
In this particular ticket, there wasn't overly a need to represent the ID as an identifier (although good job on spotting that pattern!). A simple string as the name would've sufficed, although if you wanted to later expand it you could have the id then have a "name" (and instead be id.name rather than id.id).
| counterClient.resetCounter( | ||
| { id: { id: 'test_counter' } }, | ||
| (err, resp) => { | ||
| expect(err).toBeNull(); |
There was a problem hiding this comment.
I would make sure you have a part before the resetCounter to explicitly set the value to something other than zero. There's a chance a previous test already sets the counter to zero, and there's a chance this runs before any other tests made modifications, giving a false positive
|
|
||
| it('increment counter 2 ', async () => { | ||
| const promise = new Promise((resolve) => { | ||
| counterClient.incrementCounter( |
There was a problem hiding this comment.
Similarly to below comment, any test should not be reliant on another test to operate. You could instead have increment happen twice within this it block.
| @@ -1,7 +1,7 @@ | |||
| // Code generated by protoc-gen-ts_proto. DO NOT EDIT. | |||
| // versions: | |||
| // protoc-gen-ts_proto v1.181.2 | |||
| // protoc v5.28.2 | |||
| // protoc v5.28.3 | |||
There was a problem hiding this comment.
nit: Because you installed a slightly different version (5.28.3 vs 5.28.2), it causes all of the generated proto files to change in your changelog. To avoid these files getting constantly recommitted, I would recommend either switching to the version in the repository or not committing these files if no changes were made
-should work lets hope