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

chore(init-templates): allow renaming in templates for cdk migrate apps #27166

Merged
merged 49 commits into from
Oct 11, 2023

Conversation

HBobertz
Copy link
Contributor

@HBobertz HBobertz commented Sep 15, 2023

New integ test for the new CDK migrate command. Template.txt is a sample cloudformation template with enough complexity to be a good enough sample.

Tests for Java and Csharp aren't there because they are broken right now. Add them back after we fix


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

@aws-cdk-automation aws-cdk-automation requested a review from a team September 15, 2023 17:17
@github-actions github-actions bot added the p2 label Sep 15, 2023
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Sep 15, 2023
@HBobertz HBobertz marked this pull request as ready for review September 15, 2023 17:19
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 15, 2023
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

I'm thinking this should actually be placed within the cli-integ-tests folder. It could even go in the cli.integtest.ts file because it is a CLI test.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 15, 2023
@HBobertz
Copy link
Contributor Author

HBobertz commented Sep 15, 2023

I'm thinking this should actually be placed within the cli-integ-tests folder. It could even go in the cli.integtest.ts file because it is a CLI test.

I had planned to separate them because it seems any test that uses cdk init is separated from the other commands. I didn't know if this was due to a compatibility issue, runtime optimization, or if it was just arbitrary. But I had planned to keep that structure just in case. If that's not right, then I'm happy to put it in the cli-integ-tests suite because it would definitely simplify the process.

Of note any test that uses cdk init (including our migrate command) seems to fail locally, where as the others seem to succeed locally so I had assumed these suites were separated for compatibility reasons. Unsure if this is just an issue with my environment or it's an issue with init being testable locally

@TheRealAmazonKendra
Copy link
Contributor

Looking a bit more closely, I do think that we should put this in the cli.integtest.ts file and then reuse the setup already being used in there so that we can actually do a deployment of the synthesized app. I know we had discussed only testing as far as synthesis goes before, but since, in this case we know we're starting with good templates, I think we should actually deploy it and verify that deployment.

@HBobertz
Copy link
Contributor Author

Looking a bit more closely, I do think that we should put this in the cli.integtest.ts file and then reuse the setup already being used in there so that we can actually do a deployment of the synthesized app. I know we had discussed only testing as far as synthesis goes before, but since, in this case we know we're starting with good templates, I think we should actually deploy it and verify that deployment.

will do

@HBobertz HBobertz closed this Sep 15, 2023
@HBobertz
Copy link
Contributor Author

HBobertz commented Sep 15, 2023

Was gonna close but I'll just push a commit. That'll be significantly easier.

@HBobertz HBobertz reopened this Sep 15, 2023
@HBobertz HBobertz changed the title chore: add dummy test for future canary testing chore: add CDK migrate CLI integ test Sep 18, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

template.txt 🫣

Copy link
Contributor Author

@HBobertz HBobertz Sep 18, 2023

Choose a reason for hiding this comment

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

lol want it to be .json? or just a more descriptive name than "template"

Copy link
Contributor

Choose a reason for hiding this comment

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

Both!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed them

@@ -50,6 +50,31 @@ integTest('VPC Lookup', withDefaultFixture(async (fixture) => {
await fixture.cdkDeploy('import-vpc', { modEnv: { ENABLE_VPC_TESTING: 'IMPORT' } });
}));

integTest(
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's synth and deploy this in each target language and then verify the deployment with something like

// verify the the stack
const response = await fixture.aws.cloudFormation('describeStacks', {
    StackName: stackArn,
  });
  expect(response.StackName).toEqual(stackName);

As the test is written, it's not testing anything beyond what our unit tests already cover.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I'd move it below the tests that actually test synth and deploy.

const tempPath = path.resolve(context.integTestDir);
const inputFile = path.join(__dirname, 'template.txt');

await tempShell.shell([
Copy link
Contributor

Choose a reason for hiding this comment

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

This can't possibly pass. It's missing --stack-name.

Copy link
Contributor Author

@HBobertz HBobertz Sep 18, 2023

Choose a reason for hiding this comment

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

Hm good point. It's actually kind of alarming because this did pass, and that likely means our other init tests aren't being tested currently. Since this is identical to how we do our init tests today

Copy link
Contributor

Choose a reason for hiding this comment

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

Most of those tests have an expect statement of some sort. I see that not all do but I'm not totally familiar with their inner workings.

@TheRealAmazonKendra
Copy link
Contributor

What's the error you're getting for csharp and java?

@HBobertz
Copy link
Contributor Author

What's the error you're getting for csharp and java?

Java

  [ERROR] COMPILATION ERROR : 
  [ERROR] /private/var/folders/7z/jm3q4jws2mdcjw47_sswyw0m0000gr/T/cdk-integ-0bmx1jjif5c/cdktest-0bmx1jjif5c-java-migrate-stack/src/main/java/com/myorg/Cdktest0Bmx1Jjif5CJavaMigrateStackStack.java:[14,1] duplicate class: com.acme.test.simple.Cdktest0Bmx1Jjif5CJavaMigrateStackStack
  [ERROR] /private/var/folders/7z/jm3q4jws2mdcjw47_sswyw0m0000gr/T/cdk-integ-0bmx1jjif5c/cdktest-0bmx1jjif5c-java-migrate-stack/src/main/java/com/myorg/Cdktest0Bmx1Jjif5CJavaMigrateStackApp.java:[13,13] cannot access com.myorg.Cdktest0Bmx1Jjif5CJavaMigrateStackStack
    bad source file: /private/var/folders/7z/jm3q4jws2mdcjw47_sswyw0m0000gr/T/cdk-integ-0bmx1jjif5c/cdktest-0bmx1jjif5c-java-migrate-stack/src/main/java/com/myorg/Cdktest0Bmx1Jjif5CJavaMigrateStackStack.java
      file does not contain class com.myorg.Cdktest0Bmx1Jjif5CJavaMigrateStackStack
      Please remove or make sure it appears in the correct subdirectory of the sourcepath.
  [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project cdktest-0bmx1jjif5c-java-migrate-stack: Compilation failure: Compilation failure: 
  [ERROR] /private/var/folders/7z/jm3q4jws2mdcjw47_sswyw0m0000gr/T/cdk-integ-0bmx1jjif5c/cdktest-0bmx1jjif5c-java-migrate-stack/src/main/java/com/myorg/Cdktest0Bmx1Jjif5CJavaMigrateStackStack.java:[14,1] duplicate class: com.acme.test.simple.Cdktest0Bmx1Jjif5CJavaMigrateStackStack
  [ERROR] /private/var/folders/7z/jm3q4jws2mdcjw47_sswyw0m0000gr/T/cdk-integ-0bmx1jjif5c/cdktest-0bmx1jjif5c-java-migrate-stack/src/main/java/com/myorg/Cdktest0Bmx1Jjif5CJavaMigrateStackApp.java:[13,13] cannot access com.myorg.Cdktest0Bmx1Jjif5CJavaMigrateStackStack
  [ERROR]   bad source file: /private/var/folders/7z/jm3q4jws2mdcjw47_sswyw0m0000gr/T/cdk-integ-0bmx1jjif5c/cdktest-0bmx1jjif5c-java-migrate-stack/src/main/java/com/myorg/Cdktest0Bmx1Jjif5CJavaMigrateStackStack.java
  [ERROR]     file does not contain class com.myorg.Cdktest0Bmx1Jjif5CJavaMigrateStackStack
  [ERROR]     Please remove or make sure it appears in the correct subdirectory of the sourcepath.
  [ERROR] -> [Help 1]
  org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project cdktest-0bmx1jjif5c-java-migrate-stack: Compilation failure

Csharp

  /private/var/folders/7z/jm3q4jws2mdcjw47_sswyw0m0000gr/T/cdk-integ-0q452a90fgkm/cdktest-0q452a90fgkm-csharp-migrate-stack/src/Cdktest0Q452A90FgkmCsharpMigrateStack/Cdktest0Q452A90FgkmCsharpMigrateStackStack.cs(2,22): error CS0234: The type or namespace name 'dynamodb' does not exist in the namespace 'Amazon.CDK.AWS' (are you missing an assembly reference?) [/private/var/folders/7z/jm3q4jws2mdcjw47_sswyw0m0000gr/T/cdk-integ-0q452a90fgkm/cdktest-0q452a90fgkm-csharp-migrate-stack/src/Cdktest0Q452A90FgkmCsharpMigrateStack/Cdktest0Q452A90FgkmCsharpMigrateStack.csproj]
  /private/var/folders/7z/jm3q4jws2mdcjw47_sswyw0m0000gr/T/cdk-integ-0q452a90fgkm/cdktest-0q452a90fgkm-csharp-migrate-stack/src/Cdktest0Q452A90FgkmCsharpMigrateStack/Cdktest0Q452A90FgkmCsharpMigrateStackStack.cs(10,87): error CS0246: The type or namespace name 'Cdktest0Q452A90FgkmCsharpMigrateStackStackProps' could not be found (are you missing a using directive or an assembly reference?) [/private/var/folders/7z/jm3q4jws2mdcjw47_sswyw0m0000gr/T/cdk-integ-0q452a90fgkm/cdktest-0q452a90fgkm-csharp-migrate-stack/src/Cdktest0Q452A90FgkmCsharpMigrateStack/Cdktest0Q452A90FgkmCsharpMigrateStack.csproj]

@TheRealAmazonKendra TheRealAmazonKendra temporarily deployed to test-pipeline October 10, 2023 03:00 — with GitHub Actions Inactive
@TheRealAmazonKendra TheRealAmazonKendra temporarily deployed to test-pipeline October 10, 2023 21:19 — with GitHub Actions Inactive
@TheRealAmazonKendra TheRealAmazonKendra temporarily deployed to test-pipeline October 10, 2023 22:56 — with GitHub Actions Inactive
@TheRealAmazonKendra TheRealAmazonKendra temporarily deployed to test-pipeline October 11, 2023 00:57 — with GitHub Actions Inactive
@TheRealAmazonKendra TheRealAmazonKendra temporarily deployed to test-pipeline October 11, 2023 03:03 — with GitHub Actions Inactive
@TheRealAmazonKendra TheRealAmazonKendra changed the title chore: add CDK migrate CLI integ test chore(init-templates): allow renaming in templates for cdk migrate apps Oct 11, 2023
@TheRealAmazonKendra TheRealAmazonKendra temporarily deployed to test-pipeline October 11, 2023 04:56 — with GitHub Actions Inactive
@TheRealAmazonKendra TheRealAmazonKendra temporarily deployed to test-pipeline October 11, 2023 22:48 — with GitHub Actions Inactive
@TheRealAmazonKendra TheRealAmazonKendra added pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested and removed pr/do-not-merge This PR should not be merged at this time. labels Oct 11, 2023
@TheRealAmazonKendra TheRealAmazonKendra temporarily deployed to test-pipeline October 11, 2023 22:53 — with GitHub Actions Inactive
@aws-cdk-automation aws-cdk-automation dismissed their stale review October 11, 2023 22:54

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

@aws-cdk-automation aws-cdk-automation removed the pr/needs-cli-test-run This PR needs CLI tests run against it. label Oct 11, 2023
@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.

@mergify
Copy link
Contributor

mergify bot commented Oct 11, 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
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 52d024b
  • 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 6143b16 into main Oct 11, 2023
8 of 9 checks passed
@mergify mergify bot deleted the bobertzh/testing branch October 11, 2023 23:49
@mergify
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2 pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants