feat: Kubernetes changes #192
Conversation
okan-cakmak
force-pushed
the
kubernetes-changes
branch
from
ab6ae17 to
8fe4297
Compare
okan-cakmak
force-pushed
the
kubernetes-changes
branch
from
8fe4297 to
1be600a
Compare
| env: | ||
| GH_TOKEN: ${{ secrets.GH_PERSONAL_ACCESS_TOKEN }} | ||
|
|
||
| post-deploy: |
There was a problem hiding this comment.
As far as I see, this is similar to scim. Can you remind me why we need these in this file? I mean this could be a dev publish. (@yuqu)
There was a problem hiding this comment.
If this is a dev publish it will just create release markers on Newrelic and Sentry for dev environment, we won't see it for prod environment
There was a problem hiding this comment.
Newrelic shows "related changes" as markers, I'd definitely not want to see it as a related change in its prod version, for example.
Also, the reason I was confused was because I forgot we'll get rid off development.yaml and production.yaml files. We should check if the new docker publish does everything in those files in other services, too (for example public-api does not put markets in the docker publish file).
| @@ -48,12 +48,13 @@ | |||
| "express": "^4.17.1", | |||
| "helmet": "7.0.0", | |||
| "http-status-codes": "^1.4.0", | |||
| "http-terminator": "^3.2.0", | |||
| "mixpanel": "^0.13.0", | |||
| "mongoose": "7.6.7", | |||
| "newrelic": "8.14.0", | |||
There was a problem hiding this comment.
Why don't we upgrade newrelic? (and maybe axios?)
There was a problem hiding this comment.
upgrading newrelic throws this error. so I've skipped it to fix with the node 20 changes
There was a problem hiding this comment.
OK 👍 We should not need it for Node 16 anyways.
| @@ -28,6 +29,8 @@ interface SentryGetParams { | |||
| interface ErrorTracker { | |||
| captureError: (error: ServerError, extra: Extra) => void; | |||
| flush: () => Promise<void> ; | |||
| close: (timeout?: number) => Promise<boolean>; | |||
| SENTRY_CLOSE_TIMEOUT?: number; | |||
There was a problem hiding this comment.
Looks like this is a generic setup. I suggest calling this as CLOSE_TIMEOUT .
src/server/app.ts
Outdated
| const httpTerminator = createHttpTerminator({ server: this.server }); | ||
| await httpTerminator.terminate(); | ||
|
|
||
| delete this.server; |
There was a problem hiding this comment.
This is something we didn't have in other services. Why do need it and should we change the others accordingly?
There was a problem hiding this comment.
It is just a convenience operation, to not attempt to close the server if already done.
the existing codebase is improved in scim, ms-teams etc. to do such things in an elegant way. For example, server in zeplin-api is defined as let server; on file scope, we cannot use delete operator on such a variable, but we could set it to undefined. As I said, it is not mandatory but just a nice to have thing
There was a problem hiding this comment.
Also, in some of the other services, we did initialized the httpTerminator on server.init. Does it have a difference?
There was a problem hiding this comment.
according to the source code, it makes difference. it keeps the list of open sockets while server is listening.
| @@ -48,12 +48,13 @@ | |||
| "express": "^4.17.1", | |||
| "helmet": "7.0.0", | |||
| "http-status-codes": "^1.4.0", | |||
| "http-terminator": "^3.2.0", | |||
| "mixpanel": "^0.13.0", | |||
| "mongoose": "7.6.7", | |||
| "newrelic": "8.14.0", | |||
There was a problem hiding this comment.
OK 👍 We should not need it for Node 16 anyways.
package.json
Outdated
| @@ -4,7 +4,7 @@ | |||
| "description": "", | |||
| "main": "index.js", | |||
| "scripts": { | |||
| "dev": "NODE_ENV=development nodemon", | |||
| "dev": "NODE_ENV=development NEW_RELIC_APP_NAME=msteams-local nodemon", | |||
Change description
Type of change
Related issues
Checklists
Development
Security
Code review