Skip to content
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

feat(cli): support Fn::ImportValue intrinsic function for hotswap deployments #27292

Merged

Conversation

tomwwright
Copy link
Contributor

@tomwwright tomwwright commented Sep 26, 2023

Purpose 🎯

Extend the EvaluateCloudFormationTemplate class to support the Fn::ImportValue intrinsic function. This allows for more diverse templates to be evaluated for the purposes of determining eligibility for --hotswap deployments

Closes #21320

Approach 🧠

Implement LazyLookupExport in similar fashion to LazyListStackResources to cache required CloudFormation API calls (preference was to implement using a generator function instead so style is not entirely consistent, is this an issue?)

Add some basic unit tests for EvaluateCloudFormationTemplate.evaluateCfnExpression() is they were absent, then add some tests for Fn::ImportValue

Todo 📝

  • Update doco where appropriate
  • Add to hotswap deployment tests
  • Look for appropriate integration tests to update

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

@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 labels Sep 26, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team September 26, 2023 07:29
@tomwwright tomwwright changed the title feat(cdk): feat(cdk): support Fn::ImportValue intrinsic function for hotswap deployments Sep 26, 2023
@tomwwright tomwwright changed the title feat(cdk): support Fn::ImportValue intrinsic function for hotswap deployments feat(cdk): support Fn::ImportValue intrinsic function for hotswap deployments Sep 26, 2023
@tomwwright tomwwright force-pushed the tomwwright/support-fn-importvalue-in-evaluate branch from 0a78d8a to ecdd7c7 Compare September 26, 2023 07:30
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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.

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

@tomwwright
Copy link
Contributor Author

Exception Request: Features must contain a change to an integration test file and the resulting snapshot.

I've reviewed the integration tests for packages/aws-cdk and I don't see a relevant test to update -- can someone indicate which would be appropriate, or confirm that this feat doesn't touch any?

@tomwwright
Copy link
Contributor Author

Clarification Request: CLI code has changed. A maintainer must run the code through the testing pipeline

I'm not certain on how to action this one -- could someone help me get this linter request resolved?

@aws-cdk-automation aws-cdk-automation added the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Sep 27, 2023
@tomwwright tomwwright force-pushed the tomwwright/support-fn-importvalue-in-evaluate branch from 2b5af44 to 97784ab Compare September 28, 2023 00:32
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 28, 2023
packages/aws-cdk/README.md Outdated Show resolved Hide resolved
@tomwwright
Copy link
Contributor Author

Have rebased to tip of main to resolve conflicts

@tomwwright tomwwright force-pushed the tomwwright/support-fn-importvalue-in-evaluate branch from b9d9c7c to 12e9c6a Compare October 17, 2023 22:54
@tomwwright
Copy link
Contributor Author

@vinayak-kukreja added the doco link and rebased to tip of main to update -- let me know if there was anything else pending!

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@vinayak-kukreja vinayak-kukreja changed the title feat(cdk): support Fn::ImportValue intrinsic function for hotswap deployments feat(cli): support Fn::ImportValue intrinsic function for hotswap deployments Oct 18, 2023
@vinayak-kukreja
Copy link
Contributor

Hey @tomwwright , the PR looks good to me. Running it through our test pipeline again with the merge from main. Will approve the PR if the workflow passes.

@vinayak-kukreja vinayak-kukreja temporarily deployed to test-pipeline October 18, 2023 15:29 — with GitHub Actions Inactive
@vinayak-kukreja vinayak-kukreja added pr-linter/do-not-close The PR linter will not close this PR while this label is present pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Oct 18, 2023
@vinayak-kukreja
Copy link
Contributor

Hey apologies for the delay. We are facing some issues with our test pipeline. I will update here as I get a chance to run this PR through the pipeline.

@vinayak-kukreja vinayak-kukreja temporarily deployed to test-pipeline October 19, 2023 14:41 — with GitHub Actions Inactive
@aws-cdk-automation
Copy link
Collaborator

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

@vinayak-kukreja vinayak-kukreja added pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested and removed pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run pr/needs-cli-test-run This PR needs CLI tests run against it. labels Oct 19, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review October 19, 2023 16:22

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

Copy link
Contributor

@vinayak-kukreja vinayak-kukreja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @tomwwright , the pipeline run was successful. Thank you for contributing to the cdk. Really appreciate your effort.

@mergify
Copy link
Contributor

mergify bot commented Oct 19, 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).

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Oct 19, 2023
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 79cb2c7
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit a54ea0f into aws:main Oct 19, 2023
10 of 11 checks passed
@mergify
Copy link
Contributor

mergify bot commented Oct 19, 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).

@tomwwright
Copy link
Contributor Author

Thank you for your review @vinayak-kukreja 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested pr-linter/do-not-close The PR linter will not close this PR while this label is present pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

core(cli): hotswap to be more tolerant of the Fn::ImportValue usage
5 participants