-
Notifications
You must be signed in to change notification settings - Fork 47
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
43 Deployment #44
43 Deployment #44
Conversation
extension/k8s/values.yaml
Outdated
path: '' | ||
|
||
|
||
readinessProbe: |
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.
Do we have a TODO for that?
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.
} catch (e) { | ||
console.error('Error when creating interface interaction custom type, skipping...', JSON.stringify(e)) | ||
logger.error('Error when creating interface interaction custom type, skipping...', JSON.stringify(e)) |
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.
Do not stringify an error. Proper format with bunyan would be:
logger.error(e, "My custom message")
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 also check al other places in the code.
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.
await ctpClient.create(ctpClient.builder.types, paymentCustomType) | ||
logger.info('Successfully created payment custom type') |
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 the logger also outputs the tenant info?
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.
Right now we don't have multitenancy support, so it doesn't have tenant info.
notification/Dockerfile
Outdated
@@ -0,0 +1,13 @@ | |||
FROM mhart/alpine-node:10 |
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.
Just an info: GCP functions support currently only following node versions: 6, 8
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.
node 10 is now in beta: https://cloud.google.com/functions/docs/concepts/nodejs-10-runtime
notification/k8s/values.yaml
Outdated
# memory: '1Gi' #equivalent to 2GiB | ||
# limits: | ||
# cpu: '300m' #30% of CPU time on 1 GCP Core | ||
# memory: '1Gi' #equivalent to 2GB |
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 no resources requests/limits are set?
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.
We are in a faze of figuring it out 🙂
}, | ||
"required": true, | ||
"type": { | ||
"name": "String" |
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 is also date/time type. Is String on purpose here?
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.
if (body.results.length === 0) { | ||
logger.debug('Creating interface interaction') | ||
await ctpClient.create(ctpClient.builder.types, interfaceInteractionType) | ||
logger.info('Successfully created an interfaceInteraction type') |
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 output the key of the type 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.
As stated above, I would change it in a different task: #8
break | ||
} catch (err) { | ||
} catch (e) { |
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.
Pls add a test for this catch error case.
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 is a test and it was failing. Fixed the issue. Thanks
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.
GJ 👍
Fixes: #43