-
Notifications
You must be signed in to change notification settings - Fork 34
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
Align CDK Template #1577
Align CDK Template #1577
Conversation
🦋 Changeset detectedLatest commit: f479b47 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
… upgrade-cdk-template
… upgrade-cdk-template
d904752
to
2a2adfe
Compare
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 a few questions on the existing Serverless template, feel free to merge if we'd rather tackle later/elsewhere.
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 think this sample code in particular is useful? I don't have a strong opinion, but while code like src/framework/logging.ts
is meant to be usable in your actual app, this is basically code that demonstrates how you could structure a worker, that you will need to delete from the repo afterward.
It then makes me sad when I see it hanging around 😅 as dead code: https://github.com/search?q=org%3ASEEK-Jobs%20path%3Asrc%2Fservices%2FjobScorer.ts&type=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.
Think we can address these all in another PR
spy: () => { | ||
jest.spyOn(logging.logger, 'error').mockImplementation(logger.error); | ||
jest.spyOn(logging.logger, 'info').mockImplementation(logger.info); | ||
jest.spyOn(logging.logger, 'debug').mockImplementation(logger.debug); | ||
}, |
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.
Question, do we like this pattern? Would we rather
- Have some kind of default automock, to reduce boilerplate?
- Investigate a different option to test logging? Recently I've been playing with a custom destination like
createLogger({}, { write: (msg) => (myFakeStdout += msg)}
because it's simple and gives you full coverage oflogger.child
,@seek/logger
trimming, etc.
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.
Ref regarding 2: SEEK-Jobs/indie-hirer-posting-preferences-api#44
addLayers: false, | ||
enableDatadogLogs: false, | ||
flushMetricsToLogs: false, | ||
extensionLayerVersion: 58, |
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.
🤔 should I be concerned about this?
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.
II was planning on setting up a customManager in rynovate under infra/.*ts
to renovate these.
Awaiting DataDog/datadog-cdk-constructs#303 |
Can you add README file similar to other templates? |
|
||
import { config } from './config'; | ||
|
||
// Updated by https://github.com/seek-oss/rynovate |
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.
Ripped out changes from: #1518