fix(cli): add Cloud Assembly backwards compat tests #4625

rix0rrr · 2019-10-22T10:08:38Z

Make sure that the CLI will continue to be able to load old cloud
assemblies. We do this by using the CLI feature that it can directly
load Cloud Assemblies through its --app argument, hard copying
a couple of cloud assemblies into the test set and trying to synth
those.

In addition to adding a test for loading v0.36.0 assemblies, also
add tests for 2 common context provider requests (AZs and VPC),
to make sure new CLIs will continue to be able to handle these
context lookup requests.

Fixes #4475, pre-emptively adds a regression test for #4544.

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

Make sure that the CLI will continue to be able to load old cloud assemblies. We do this by using the CLI feature that it can directly load Cloud Assemblies through its `--app` argument, hard copying a couple of cloud assemblies into the test set and trying to synth those. In addition to adding a test for loading v0.36.0 assemblies, also add tests for 2 common context provider requests (AZs and VPC), to make sure new CLIs will continue to be able to handle these context lookup requests. Fixes #4475, pre-emptively adds a regression test for #4544.

mergify · 2019-10-22T10:09:15Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-10-22T10:54:15Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

nija-at

Am I right in assuming that at least one test case will need to be added here for every cx-api version bump? If so, should we enforce that, i.e. fail if cx-api version is > 1.10?

add tests for 2 common context provider requests (AZs and VPC)

I'm not familiar with these, so someone familiar should take a look as well.

nija-at · 2019-10-22T12:20:14Z

packages/aws-cdk/test/integ/cli/test-backwards-compat.sh

+tmpdir=$(dirname $(mktemp -u))
+casmdir=$tmpdir/cdk-integ-cx


Could just be casmdir=$(mktemp -d). Everything else should work AFAICT

But no. I want a predictable dir, for the following reasons:

If I do a random dir every time, I have to clean it up after every test.

If I have to clean it up after every test, I can't inspect the state of the directory after a failing test.

Instead, I have a directory that's the same every time which gets reset every time.

This is just a complex way of writing casmdir=/tmp/cdk-integ-cx, but in a way that is compatible across systems where the tempdir doesn't necessarily live at /tmp (for example MacOS or Windows).

nija-at · 2019-10-22T12:22:46Z

packages/aws-cdk/test/integ/cli/test-backwards-compat.sh

+    echo "ASSEMBLY ${1}"
+    rm -rf $casmdir
+    mkdir -p $casmdir
+    cp -R $1/* $casmdir


If you instead do a mktemp each time, instead of clobbering, we would retain (temporarily) all of the artifacts, making it easier to debug when they fail.

Not sure I follow. If a test succeeds, the artifacts can be clobbered, if a test fails they won't be?

nija-at · 2019-10-22T12:30:26Z

packages/aws-cdk/test/integ/cli/test-backwards-compat.sh

+    for template in $casmdir/*.js; do
+        node $template > ${template%.js}
+    done


Avoid loops in bash where possible; they are not very efficient but more importantly unnatural to shell scripting.

Instead use pipes. Something like, ls -1 $casmdir/*.js | xargs -I{} node {} ...

Please don't make me do that.

This is readable, writable and clear. All that find and xargs magic makes it horrible and unreadable. I dare you to write the same command with xargs, including the shell redirect to individual files. Show me what it looks like and we'll compare quality.

For the records, for loops have been natural to my shell scripting since longer than I can remember...

nija-at · 2019-10-22T12:31:59Z

packages/aws-cdk/test/integ/cli/test-backwards-compat.sh

+# tempdir. Evaluate .js files found their (interpret them
+# as templates).
+function prepare_cloud_assembly() {
+    echo "ASSEMBLY ${1}"


Minor: Ideally assign $1 at the top to a local var with an appropriate name and then use them

local sourcedir=$1

Better readability when there are no named parameters.

nija-at · 2019-10-22T12:39:15Z

packages/aws-cdk/test/integ/cli/test-backwards-compat.sh

+# Echo the TEST_ACCOUNT, TEST_REGION vars to make bash abort if they're not set.
+echo "Running backwards compatibility test (account ${TEST_ACCOUNT}, region ${TEST_REGION})"
+
+for assembly in ${scriptdir}/cloud-assemblies/*; do


Same comment as the for loop, but I understand you might want to keep this one for readability.

If it were up to me, I would group what is in the loop into a function and then use pipes and xargs. I'll leave it to you to decide.

I don't think this code can be conveniently rewritten to use xargs.

nija-at · 2019-10-22T12:45:15Z

packages/aws-cdk/test/integ/cli/cloud-assemblies/0.36.0/cdk.out

@@ -0,0 +1 @@
+{"version":"0.36.0"}


A bit confused - Isn't cdk.out a directory that contains the manifest and template?

Yes it is. I think this file exists for backwards compatibility with some old version. Elad will know.

I think that is correct.

Indeed. It is there since old CLIs would look for this file name in the output directory. The version field here will tell them that they can't do anything besides yelling. I assume we can probably get rid of it by now...

nija-at · 2019-10-22T14:23:57Z

packages/aws-cdk/test/integ/cli/test-backwards-compat.sh

+        cat $stderrfile >&2
+        echo "There was an error executing the context provider!" >&2


I don't think the messaging include which test case failed; and we should include it.

Also, it's good practice to put a sample of the output when tests fail into the PR description; just to validate that the messaging looks good.

aws-cdk-automation · 2019-10-23T08:46:34Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb

I really like this approach! How can we make sure that whenever we do a bump of the protocol version we add another "old" cloud assembly to the list?

eladb · 2019-10-25T08:43:29Z

packages/aws-cdk/test/integ/cli/cloud-assemblies/0.36.0/cdk.out

@@ -0,0 +1 @@
+{"version":"0.36.0"}


Indeed. It is there since old CLIs would look for this file name in the output directory. The version field here will tell them that they can't do anything besides yelling. I assume we can probably get rid of it by now...

RomainMuller · 2019-10-25T08:43:24Z

packages/aws-cdk/test/integ/cli/cloud-assemblies/0.36.0/cdk.out

@@ -0,0 +1 @@
+{"version":"0.36.0"}


I think that is correct.

Stale

mergify · 2019-10-25T11:54:06Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2019-10-25T12:44:39Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-25T13:01:02Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

rix0rrr requested review from RomainMuller, skinny85, eladb and nija-at October 22, 2019 10:08

rix0rrr self-assigned this Oct 22, 2019

nija-at added the pr/do-not-merge This PR should not be merged at this time. label Oct 22, 2019

nija-at previously requested changes Oct 22, 2019

View reviewed changes

SomayaB added in-progress This issue is being actively worked on. contribution/core This is a PR that came from AWS. and removed in-progress This issue is being actively worked on. labels Oct 22, 2019

Address review comments

ba04332

eladb approved these changes Oct 25, 2019

View reviewed changes

RomainMuller approved these changes Oct 25, 2019

View reviewed changes

packages/aws-cdk/test/integ/cli/cloud-assemblies/0.36.0/cdk.out

@@ -0,0 +1 @@

{"version":"0.36.0"}

Copy link

Contributor

RomainMuller Oct 25, 2019

Choose a reason for hiding this comment

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

I think that is correct.

rix0rrr requested a review from nija-at October 25, 2019 08:49

rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Oct 25, 2019

mergify bot added 2 commits October 25, 2019 11:54

Merge branch 'master' into huijbers/cli-cx-backwards-compat

8945b4b

Merge branch 'master' into huijbers/cli-cx-backwards-compat

7f0421c

mergify bot merged commit 5d2e5e3 into master Oct 25, 2019

mergify bot deleted the huijbers/cli-cx-backwards-compat branch October 25, 2019 13:01

sumodgeorge mentioned this pull request May 12, 2024

[Snyk] Security upgrade verdaccio from 3.8.1 to 5.31.0 sumodgeorge/aws-cdk#31

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): add Cloud Assembly backwards compat tests #4625

fix(cli): add Cloud Assembly backwards compat tests #4625

rix0rrr commented Oct 22, 2019

mergify bot commented Oct 22, 2019

aws-cdk-automation commented Oct 22, 2019

nija-at left a comment

nija-at Oct 22, 2019

rix0rrr Oct 23, 2019

nija-at Oct 22, 2019

rix0rrr Oct 23, 2019

nija-at Oct 22, 2019

rix0rrr Oct 23, 2019

RomainMuller Oct 25, 2019

nija-at Oct 22, 2019

nija-at Oct 22, 2019

rix0rrr Oct 23, 2019

nija-at Oct 22, 2019

rix0rrr Oct 23, 2019

RomainMuller Oct 25, 2019

eladb Oct 25, 2019

nija-at Oct 22, 2019

aws-cdk-automation commented Oct 23, 2019

eladb left a comment

eladb Oct 25, 2019

RomainMuller Oct 25, 2019

mergify bot commented Oct 25, 2019

aws-cdk-automation commented Oct 25, 2019

aws-cdk-automation commented Oct 25, 2019

		cat $stderrfile >&2
		echo "There was an error executing the context provider!" >&2

		tmpdir=$(dirname $(mktemp -u))
		casmdir=$tmpdir/cdk-integ-cx

fix(cli): add Cloud Assembly backwards compat tests #4625

fix(cli): add Cloud Assembly backwards compat tests #4625

Conversation

rix0rrr commented Oct 22, 2019

mergify bot commented Oct 22, 2019

aws-cdk-automation commented Oct 22, 2019

AWS CodeBuild CI Report

nija-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 23, 2019

AWS CodeBuild CI Report

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Oct 25, 2019

aws-cdk-automation commented Oct 25, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 25, 2019

AWS CodeBuild CI Report