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.

Sign up for GitHub

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

Jump to bottom

fix(efs): FileSystem deployment fails if vpcSubnets is changed #26155

Merged

mergify merged 2 commits into aws:main from ahmetsoykan:25099

Aug 22, 2023

Contributor

ahmetsoykan commented Jun 28, 2023 •

edited by rix0rrr

Loading

Changing Logical IDs for EfsMountTarget

While using the forEach, index will be added as a suffix to logical id of EfsMountTarget
this is causing an error when the subnets array in the props has changed.

Closes #25099

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io bot commented Jun 28, 2023

github-actions bot added repeat-contributor bug effort/small p2 labels

aws-cdk-automation requested a review from a team

June 28, 2023 20:27

aws-cdk-automation previously requested changes

View reviewed changes

Collaborator

aws-cdk-automation left a comment

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.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

ahmetsoykan changed the title ~~fix(aws-efs): change logical id for EfsMountTarget~~ fix(efs): change logical id for EfsMountTarget

ahmetsoykan requested a review from aws-cdk-automation

June 28, 2023 22:20

ahmetsoykan force-pushed the 25099 branch from 17857ca to 849fd7c Compare

June 29, 2023 19:34

aws-cdk-automation dismissed their stale review

June 30, 2023 12:00

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

aws-cdk-automation added the pr/needs-community-review label

ahmetsoykan force-pushed the 25099 branch 2 times, most recently from cc46389 to 42abb22 Compare

June 30, 2023 12:56

ahmetsoykan mentioned this pull request

fix(efs): change logical id for EfsMountTarget #25996

Closed

ahmetsoykan force-pushed the 25099 branch 4 times, most recently from d65c0ca to 4e66d80 Compare

July 7, 2023 06:32

Collaborator

aws-cdk-automation commented Jul 13, 2023

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

2 similar comments

Collaborator

aws-cdk-automation commented Jul 14, 2023

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

Collaborator

aws-cdk-automation commented Jul 15, 2023

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

ahmetsoykan force-pushed the 25099 branch from 4e66d80 to ccf2489 Compare

July 15, 2023 19:33

Collaborator

aws-cdk-automation commented Jul 19, 2023

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

ahmetsoykan force-pushed the 25099 branch 2 times, most recently from e6274d8 to e54b414 Compare

July 21, 2023 18:21

github-actions bot mentioned this pull request

Monthly PRs metrics report - June 2023 #26516

Closed

Collaborator

aws-cdk-automation commented Jul 29, 2023

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

ahmetsoykan force-pushed the 25099 branch from e54b414 to 9f2ace7 Compare

July 29, 2023 19:39

lpizzinidev reviewed

View reviewed changes

packages/aws-cdk-lib/aws-efs/test/efs-file-system.test.ts

                 expect(importedFileSystemResult).toStrictEqual({
                   statementAdded: false,
                 });
+              });
+              test('mountTargetOrderInsensitiveLogicalId flag is true', () => {

Contributor

lpizzinidev Jul 30, 2023

Can you update the test to use custom subnets to check that the subnet ID is used as a suffix?

For example:

new FileSystem(customVpc, 'EfsFileSystem', {
  vpc: customVpc,
  vpcSubnets: { subnets: [ec2.Subnet.fromSubnetId(stack, 'SubnetExample', 'subnet-1')] },
});

Contributor Author

ahmetsoykan Jul 30, 2023 •

edited

Loading

@lpizzinidev hello, when checking subnetID from created VPC, SubnetID attr tokenised and hard to correlate with template output in this test.

I think with this PR, we're actually not passing SubnetID attribute to MountTarget's logical IDs, we're passing ${subnet.node.id} which i believe sort of concatenation like ${subnetgroupname}Subnet${index}. So i'm a bit unsure passing subnet selection to FileSystem Contruct. Did you want me to do this because of some future changes that might have happened in VPC definitions in this repository cause test failure?

Isn't it a good method that checking generated logical ids of mount target from resulting different cfn templates regarding to this feature flag?

thank you for your review.

Contributor

lpizzinidev Aug 2, 2023

Thanks for the clarification.
I probably interpreted the test case in the wrong way.

lpizzinidev suggested changes

View reviewed changes

Contributor

lpizzinidev left a comment

Thanks for opening a new PR! 👍
Looks good overall, just a small change on the test.

aws-cdk-automation removed the pr/needs-community-review label

lpizzinidev approved these changes

View reviewed changes

aws-cdk-automation added pr/needs-maintainer-review and removed pr/needs-maintainer-review labels


          fix(aws-efs): change logical id for EfsMountTarget

2eecafb

fix(aws-efs): change logical id for EfsMountTarget

fix(efs): recreate integ.ecs-job-definition

fix(efs): recreate integ.ecs-job-definition

fix(efs): recreate integ.ecs-job-definition

fix(aws-efs): change logical id for EfsMountTarget

Revert "chore(release): 2.87.0"

This reverts commit 2879e47.

ahmetsoykan force-pushed the 25099 branch from 58453a4 to 2eecafb Compare

August 19, 2023 17:55

aws-cdk-automation added the pr/needs-maintainer-review label

rix0rrr approved these changes

View reviewed changes

rix0rrr changed the title ~~fix(efs): change logical id for EfsMountTarget~~ fix(efs): FileSystem deployment fails if vpcSubnets is changed

Contributor

mergify bot commented Aug 22, 2023

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).


          Merge branch 'main' into 25099

0b4541d

aws-cdk-automation removed the pr/needs-maintainer-review label

Collaborator

aws-cdk-automation commented Aug 22, 2023

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 0b4541d
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify bot merged commit d54b67d into aws:main

12 checks passed

Contributor

mergify bot commented Aug 22, 2023

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).

tbreysse mentioned this pull request

(EFS): EFS Mount Targets fail if subnets change positions within the vpc_subnets property #27397

Closed

lamebear mentioned this pull request

(EFS): Undocumented breaking change upgrading from 2.92 to 2.93+ #27728

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug effort/small p2 repeat-contributor