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
chore(override): enable passing down COPILOT env vars to CDK files #4350
Conversation
🍕 Here are the new binary sizes!
|
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.
LGTM overall
readonly appName?: string; | ||
readonly envName?: string; | ||
} | ||
|
||
export class TransformedStack extends cdk.Stack { | ||
constructor (scope: cdk.App, id: string, props?: cdk.StackProps) { | ||
constructor (scope: cdk.App, id: string, props: TransformedStackProps) { |
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 appName
and envName
are optional, why do we make props
required?
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.
good question 😂 changed to appName
and envName
being required
exec struct { | ||
LookPath func(file string) (string, error) | ||
Command func(name string, args ...string) *exec.Cmd |
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.
Will this be a problem for mock for unit test?
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.
Nope! In the unit tests we use the public constructor to define our own custom behavior:
WithCDK("some/path", CDKOpts{
LookPathFn: func(string) (string, error) { return "", errors.New("some error") }
})
Codecov Report
@@ Coverage Diff @@
## mainline #4350 +/- ##
=========================================
Coverage 70.02% 70.03%
=========================================
Files 260 260
Lines 37030 37096 +66
Branches 266 266
=========================================
+ Hits 25931 25979 +48
- Misses 9853 9869 +16
- Partials 1246 1248 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it 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.
LGTM!
Related #4208
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.