feat(appmesh): change HealthChecks to use protocol-specific union-like classes #14432
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
dfezzie
commented
Apr 28, 2021
alexbrjo
approved these changes
Apr 28, 2021
There was a problem hiding this comment.
Looks good, I like the option to set a default and great to see the code redundancy get reduced. Here's a consideration, it's possible to enforce different fields per protocol type without using factories. see the implementation of connection pooling:
- connection pool shapes that enforce the right fields at compile time
- the helper object that allows any of these shapes get accepted by the rendering function
- the L1 rendering function
The con would be that using this approach would prevent us from setting defaults.
skinny85
previously requested changes
Apr 29, 2021
There was a problem hiding this comment.
Thanks for the PR @dfezzie!
I have some comments. I'm mainly concerned with the changes to the defaults, which result in a lot of test changes. I really think we should strive to minimize changes to tests when mixing them with functional changes.
Is there any way to preserve the old values in the templates, while changing the API?
Co-authored-by: Adam Ruka <adamruka@amazon.com>
|
|
Co-authored-by: Adam Ruka <adamruka@amazon.com>
|
Hey @skinny85 I've finished the update here. Let me know if there are any other changes I need to add! |
skinny85
changed the title
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
skinny85
approved these changes
May 7, 2021
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
eladb
pushed a commit
that referenced
this pull request
May 11, 2021
* chore(cloudfront): remove the use of calculateFunctionHash (#14583) `calculateFunctionHash()` was used to compute the 'refresh token' of the custom resource for the EdgeFunction construct. This method is private to the lambda module and is deemed to be changed. Instead, use the lambda function version's logical id. The logical id of the version includes computing the function hash (among others) and is a more reliable determinant of whether the underlying function version changed. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * feat(cloudwatch): validate parameters for a metric dimensions (closes #3116) (#14365) As per #3116, the changes in this PR validate metric dimension values (length, and checking if the value is null or undefined) and throw errors if the values are not valid. I've also corrected a comment in the metric-types.ts to use the correct method name * feat(appmesh): change HealthChecks to use protocol-specific union-like classes (#14432) BREAKING CHANGE: HealthChecks require use of static factory methods fixes #11640 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* Co-authored-by: Niranjan Jayakar <nija@amazon.com> Co-authored-by: OksanaH <34384274+OksanaH@users.noreply.github.com> Co-authored-by: Dominic Fezzie <fezzid@amazon.com> Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>
eladb
pushed a commit
that referenced
this pull request
May 12, 2021
* chore(cloudfront): remove the use of calculateFunctionHash (#14583) `calculateFunctionHash()` was used to compute the 'refresh token' of the custom resource for the EdgeFunction construct. This method is private to the lambda module and is deemed to be changed. Instead, use the lambda function version's logical id. The logical id of the version includes computing the function hash (among others) and is a more reliable determinant of whether the underlying function version changed. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * feat(cloudwatch): validate parameters for a metric dimensions (closes #3116) (#14365) As per #3116, the changes in this PR validate metric dimension values (length, and checking if the value is null or undefined) and throw errors if the values are not valid. I've also corrected a comment in the metric-types.ts to use the correct method name * feat(appmesh): change HealthChecks to use protocol-specific union-like classes (#14432) BREAKING CHANGE: HealthChecks require use of static factory methods fixes #11640 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * feat(msk): Cluster L2 Construct (#9908) L2 Construct for a MSK Cluster. I wrote this for internal use and thought I'd share it. I tried to follow the [example resource](https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/example-construct-library/lib/example-resource.ts) and [design guidelines](https://github.com/aws/aws-cdk/blob/master/DESIGN_GUIDELINES.md) as much as I could. Default properties were chosen either based on defaults when creating a cluster in the console or defaults set from CloudFormation. Closes #9603 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * feat(kms): allow specifying key spec and key usage (#14478) This allows specifying key spec and key usage, so you can create asymmetric keys. closes #5639 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * chore: add `@types/jest` to a package that was missing it (#14609) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * chore: issue template for bugs to have an SSCCE example (#14615) Ask customers to provide reproducible code snippets to reduce the amount of triage time required. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * fix(cli): synth fails if there was an error when synthesizing the stack (#14613) All stacks created inside a pipeline stage will be flagged for validation. After synth is done, the CLI will validate all flagged stacks plus the stacks that were explicitly specified. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * chore: annotate `aws-lambda-go` with `docker` requirement (#14618) The `nozem` build tool needs to know that `docker` is required to build/test this particular package. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * feat(elbv2): preserveClientIp for NetworkTargetGroup (#14589) Allows users to configure client IP preservation for network target groups. See: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-elasticloadbalancingv2-targetgroup-targetgroupattribute.html ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * chore: npm-check-updates && yarn upgrade (#14620) Ran npm-check-updates and yarn upgrade to keep the `yarn.lock` file up-to-date. * chore(mergify): add @BenChaimberg to team roster * chore(release): 1.103.0 * chore: mark "otaviomacedo" as core contributor (#14619) Co-authored-by: Otavio Macedo <otaviomacedo@protonmail.com> * chore(cli): add npm command to upgrade notice (#14621) A colleague had to go look up the command to update the CDK CLI. It occurred to me that is probably common with developers who don't work with NPM on a daily basis, such as anyone who isn't developing in TypeScript or JavaScript. Put the necessary command right in the upgrade notice. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * fix(lambda): custom resource fails to connect to efs filesystem (#14431) Fixes: #14430 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * feat(cfnspec): cloudformation spec v35.2.0 (#14610) * feat: cloudformation spec v35.2.0 * add spec patches Co-authored-by: AWS CDK Team <aws-cdk@amazon.com> Co-authored-by: Elad Ben-Israel <benisrae@amazon.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * fix(lambda-nodejs): handler filename missing from error message (#14564) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * chore(build): clarify prerequisites in CONTRIBUTING and verify before build (#14642) The build script at the top level of the repository performs a prerequisites check before beginning the build. This verification is not complete as it does not include the required checks for .NET and Python executables. Further, the prerequisites documentation in the contributing guide does not note the Docker requirement. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* Co-authored-by: Niranjan Jayakar <nija@amazon.com> Co-authored-by: OksanaH <34384274+OksanaH@users.noreply.github.com> Co-authored-by: Dominic Fezzie <fezzid@amazon.com> Co-authored-by: Curtis <curtis.eppel@cultureamp.com> Co-authored-by: Shinya Tsuda <shinya@dacci.org> Co-authored-by: Rico Huijbers <rix0rrr@gmail.com> Co-authored-by: Otavio Macedo <otaviomacedo@protonmail.com> Co-authored-by: Griffin Byatt <byatt.griffin@gmail.com> Co-authored-by: Ben Chaimberg <chaimber@amazon.com> Co-authored-by: AWS CDK Team <aws-cdk@amazon.com> Co-authored-by: Mitchell Valine <valinm@amazon.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Jerry Kindall <52084730+Jerry-AWS@users.noreply.github.com> Co-authored-by: Daniel Neilson <53624638+ddneilson@users.noreply.github.com> Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BREAKING CHANGE: HealthChecks require use of static factory methods
fixes #11640
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license