-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix: breaking change to deployment config props #22567
Conversation
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.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
@clareliguori I removed all of the "more specific" deployment config interfaces as part of fixing this, instead of re-implementing the Additionally we should be able to add them back with additional optional properties in the future in a non-breaking way, just keeping them around seems dangerous as it opens us to this bug of structural interfaces are implemented somewhere where an explicit type is actually needed for Java compatibility. |
Exempting tests. Our typescript tests are insufficient to test this anyhow. I will test this locally in java before merging this PR. |
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
@madeline-k pointed out a user explicitly importing and defining the more specific interfaces would be broken by this, but leaving them in feels like leaving the foot gun loaded. import { IServerDeploymentConfig } from '';
const myThing: IServerDeploymentConfig { ... }; We may have to though, @kaizencc @corymhall what do you think? |
packages/@aws-cdk/aws-codedeploy/lib/lambda/deployment-config.ts
Outdated
Show resolved
Hide resolved
Yeah I think we probably should leave them in. If we mark them as deprecated thats probably good enough to prevent them from being used in the future. |
"props-physical-name:@aws-cdk/aws-codedeploy.CustomLambdaDeploymentConfigProps", | ||
"from-signature:@aws-cdk/aws-codedeploy.EcsDeploymentConfig.fromEcsDeploymentConfigName", | ||
"from-signature:@aws-cdk/aws-codedeploy.LambdaDeploymentConfig.fromLambdaDeploymentConfigName", | ||
"from-signature:@aws-cdk/aws-codedeploy.ServerDeploymentConfig.fromServerDeploymentConfigName", | ||
"no-unused-type:@aws-cdk/aws-codedeploy.IEcsDeploymentConfig", | ||
"no-unused-type:@aws-cdk/aws-codedeploy.ILambdaDeploymentConfig", | ||
"no-unused-type:@aws-cdk/aws-codedeploy.IServerDeploymentConfig" |
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've looked over these and they seem reasonable from our changes, but want to highlight them just in case.
Change all `deploymentConfig` static method implementations on `EcsDeploymentConfig`, `LambdaDeploymentConfig`, and `ServerDeploymentConfig` to return their corresponding specific interfaces instead of `IBaseDeploymentConfig`. This reverts breaking changes to Java users introduced in #22159
ff0d4de
to
3b6bfde
Compare
Went back to keeping all of the interface types and instead of calling |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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 tested this by running a java app against my local aws-cdk-lib with this change, and did not get an error. I also tested it against a normal aws-cdk-lib and did get the error. Putting my stamp of approval on this and shipping it.
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Change all `deploymentConfig` static method implementations on `EcsDeploymentConfig`, `LambdaDeploymentConfig`, and `ServerDeploymentConfig` to return their corresponding specific interfaces instead of `IBaseDeploymentConfig`. This reverts breaking changes to Java users introduced in #22159 Fixes #22566 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Change all `deploymentConfig` static method implementations on `EcsDeploymentConfig`, `LambdaDeploymentConfig`, and `ServerDeploymentConfig` to return their corresponding specific interfaces instead of `IBaseDeploymentConfig`. This reverts breaking changes to Java users introduced in aws#22159 Fixes aws#22566 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Change all
deploymentConfig
static method implementations onEcsDeploymentConfig
,LambdaDeploymentConfig
, andServerDeploymentConfig
to return their corresponding specificinterfaces instead of
IBaseDeploymentConfig
. This reverts breakingchanges to Java users introduced in #22159
Fixes #22566
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integ
to deploy the infrastructure and generate the snapshot (i.e.yarn integ
without--dry-run
)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license