fix(opensearch): Don't throw a Az count mismatch for imported VPCs #22654
Conversation
|
|
github-actions
bot
added
beginning-contributor
stijnbrouwers
changed the title
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
Thanks, It should be OK now. I was still figuring out how to work with the integration tests :-) |
mergify
bot
dismissed
TheRealAmazonKendra’s stale review
Pull request has been modified.
The build is not... |
@Naumel |
packages/@aws-cdk/aws-opensearchservice/test/integ.opensearch.imported-vpc.ts
Outdated
Show resolved
Hide resolved
mergify
bot
dismissed
mrgrain’s stale review
Pull request has been modified.
packages/@aws-cdk/aws-opensearchservice/test/integ.opensearch.imported-vpc.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-opensearchservice/test/integ.opensearch.imported-vpc.ts
Outdated
Show resolved
Hide resolved
|
@Mergifyio update |
✅ Branch has been successfully updated |
There was a problem hiding this comment.
Code is looking good now and thanks for adding the integ test.
However the integ test does not appear to deploy successfully. Did you run it?
Context lookups have been disabled. Make sure all necessary context is already in 'cdk.context.json' by running 'cdk synth' on a machine with sufficient AWS credentials and committing the result. Missing context keys: 'vpc-provider:account=ACCOUNT:filter.tag:Name=my-vpc-name:region=us-east-1:returnAsymmetricSubnets=true'
FAILED integ.opensearch.imported-vpc-cdk-integ-opensearch-vpc-test/DefaultTest (undefined/us-east-1) 10.216s
Integration test failed: Error: Command exited with status 1
Given that the repro steps for the issue includes "clear the context" this error seems related?!
packages/@aws-cdk/aws-opensearchservice/test/integ.opensearch.imported-vpc.ts
Outdated
Show resolved
Hide resolved
mergify
bot
dismissed
mrgrain’s stale review
Pull request has been modified.
|
Yes, I ran it and it used to work After applying the app dependency change however, it failed again... Which you removed (but also doesn't seem to do anything at the moment) in integ.json. The unit test NEEDS to do a lookup, if the VPC is already in the context the bug does not occur. |
|
Thanks for the detailed response @stijnbrouwers I think I misunderstood the source of the issue. Apologies for this.
I'm not sure we can actually create this order in a single deployment (which is was the integ framework does). Because any lookups would happen before the Stack with the VPC would be deployed as well, thus failing the lookup. I'll need to think a bit more about this. This might be okay with a unit test only. |
|
@mrgrain thank you very much for your quick response. The issue that I fixed is: To mitigate this, I skip the check whenever one of the subnets has the isPendingLookup-property set to "true". Should the minimum amount of availability zones not be present, it will fail upon creation of the actual resource in AWS instead of upon creation of the cloudformation template. That should give a little insight into why I need to do the lookup in the integration test. Another solution would be to just always drop the sanity check and let AWS complain upon resource creation, but I liked this better :-) |
|
I'm with you now and I agree with the assessment to skip the check in case the lookup is pending. We don't need an integ test for this, because it's not possible to test this with the current setup. Let's remove the test and get it merged. |
mrgrain
added
the
pr-linter/exempt-integ-test
|
@mrgrain , great thanks! |
|
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). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
|
@mrgrain thank you very much for your support! |
…ws#22654) Solving issue aws#22651 Creating a domain fails for imported vpc/subnets when zone awareness is enabled and the cdk context is cleared. When the CDK context is already retrieved and the VPC is in the context, the deployment works. This is due to the fact that when there is no context yet, the subnet count is always 0. That's why I decided to disable it. If it's not correct, it will fail when applying the CloudFormation template. ---- ### 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*
…ws#22654) Solving issue aws#22651 Creating a domain fails for imported vpc/subnets when zone awareness is enabled and the cdk context is cleared. When the CDK context is already retrieved and the VPC is in the context, the deployment works. This is due to the fact that when there is no context yet, the subnet count is always 0. That's why I decided to disable it. If it's not correct, it will fail when applying the CloudFormation template. ---- ### 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*
Solving issue #22651
Creating a domain fails for imported vpc/subnets when zone awareness is enabled and the cdk context is cleared.
When the CDK context is already retrieved and the VPC is in the context, the deployment works.
This is due to the fact that when there is no context yet, the subnet count is always 0. That's why I decided to disable it. If it's not correct, it will fail when applying the CloudFormation template.
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license