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

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

Merged
merged 4 commits into from
Oct 25, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/aws-cdk/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@ coverage
*.snk

!test/integ/run-wrappers/dist
!test/integ/cli/**/*
1 change: 1 addition & 0 deletions packages/aws-cdk/test/integ/cli/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
cdk.context.json
1 change: 0 additions & 1 deletion packages/aws-cdk/test/integ/cli/app/.gitignore

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"version":"0.36.0"}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"version": "0.36.0",
"artifacts": {
"InitStack": {
"type": "aws:cloudformation:stack",
"environment": "aws://unknown-account/unknown-region",
"properties": {
"templateFile": "InitStack.template.json"
}
}
},
"runtime": {
"libraries": {
"@aws-cdk/core": "1.12.0",
"@aws-cdk/cx-api": "1.12.0",
"jsii-runtime": "node.js/v8.11.4"
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
{
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"version":"1.10.0"}
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
process.stdout.write(JSON.stringify({
"version": "1.10.0",
"artifacts": {
"InitStack": {
"type": "aws:cloudformation:stack",
"environment": `aws://${process.env.TEST_ACCOUNT}/${process.env.TEST_REGION}`,
"properties": {
"templateFile": "InitStack.template.json"
}
}
},
"runtime": {
"libraries": {
"@aws-cdk/core": "1.14.0",
"@aws-cdk/cx-api": "1.14.0",
"@aws-cdk/aws-ec2": "1.14.0",
"@aws-cdk/aws-iam": "1.14.0",
"@aws-cdk/region-info": "1.14.0",
"@aws-cdk/aws-ssm": "1.14.0",
"@aws-cdk/aws-cloudwatch": "1.14.0",
"jsii-runtime": "node.js/v8.11.4"
}
},
"missing": [
{
"key": `vpc-provider:account=${process.env.TEST_ACCOUNT}:filter.isDefault=true:region=${process.env.TEST_REGION}`,
"props": {
"account": process.env.TEST_ACCOUNT,
"region": process.env.TEST_REGION,
"filter": {
"isDefault": "true"
}
},
"provider": "vpc-provider"
}
]
}, undefined, 2));
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
{
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"version":"1.10.0"}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
process.stdout.write(JSON.stringify({
"version": "1.10.0",
"artifacts": {
"InitStack": {
"type": "aws:cloudformation:stack",
"environment": `aws://${process.env.TEST_ACCOUNT}/${process.env.TEST_REGION}`,
"properties": {
"templateFile": "InitStack.template.json"
}
}
},
"runtime": {
"libraries": {
"@aws-cdk/core": "1.14.0",
"@aws-cdk/cx-api": "1.14.0",
"@aws-cdk/aws-ec2": "1.14.0",
"@aws-cdk/aws-iam": "1.14.0",
"@aws-cdk/region-info": "1.14.0",
"@aws-cdk/aws-ssm": "1.14.0",
"@aws-cdk/aws-cloudwatch": "1.14.0",
"jsii-runtime": "node.js/v8.11.4"
}
},
"missing": [
{
"key": `availability-zones:account=${process.env.TEST_ACCOUNT}:region=${process.env.TEST_REGION}`,
"props": {
"account": process.env.TEST_ACCOUNT,
"region": process.env.TEST_REGION,
},
"provider": "availability-zones"
}
]
}, undefined, 2));
7 changes: 5 additions & 2 deletions packages/aws-cdk/test/integ/cli/common.bash
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@ scriptdir=$(cd $(dirname $0) && pwd)
cd ${scriptdir}

if [[ -z "${CREDS_SET:-}" ]]; then
# Check that credentials are configured
aws sts get-caller-identity > /dev/null
# Check that credentials are configured (will error & abort if not)
creds=$(aws sts get-caller-identity)

export TEST_ACCOUNT=$(node -p "($creds).Account")
export TEST_REGION=${AWS_REGION:-${AWS_DEFAULT_REGION:-us-east-1}}
export CREDS_SET=1
fi

Expand Down
54 changes: 54 additions & 0 deletions packages/aws-cdk/test/integ/cli/test-backwards-compat.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
#!/bin/bash
set -euo pipefail
scriptdir=$(cd $(dirname $0) && pwd)
source ${scriptdir}/common.bash
# ----------------------------------------------------------

tmpdir=$(dirname $(mktemp -u))
casmdir=$tmpdir/cdk-integ-cx
Comment on lines +7 to +8
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

stderrfile=$casmdir/cdk.err

# Copy the files from a cloud assembly source dir into the
# tempdir. Evaluate .js files found their (interpret them
# as templates).
function prepare_cloud_assembly() {
local asmdir=$1
echo "ASSEMBLY ${asmdir}"
rm -rf $casmdir
mkdir -p $casmdir
cp -R $asmdir/* $casmdir

# Execute templates to produce file with the same name
# but without .js extension
shopt -s nullglob
for template in $casmdir/*.js; do
node $template > ${template%.js}
done
Comment on lines +24 to +26
Copy link
Contributor

Choose a reason for hiding this comment

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

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 {} ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

# Assert that there was no providerError in CDK's stderr
# Because we rely on the app/framework to actually error in case the
# provider fails, we inspect the logs here.
function assert_no_error() {
local asmdir=$1

if grep '$providerError' $stderrfile; then
cat $stderrfile >&2
echo "There was an error executing the context provider for assembly ${asmdir}!" >&2
exit 1
fi
}

# 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

prepare_cloud_assembly $assembly

rm -f cdk.context.json
cdk -a $casmdir -v synth > /dev/null 2> $stderrfile

assert_no_error $assembly
done

echo "✅ success"
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,13 @@ source ${scriptdir}/common.bash
setup

parameterName=/does/not/exist
account=$(node -p "($(aws sts get-caller-identity)).Account")
region=${AWS_REGION:-${AWS_DEFAULT_REGION:-us-east-1}}

function cdk_synth() {
(cdk synth $@ 2>&1 || true) | strip_color_codes
}

assert "cdk_synth ${STACK_NAME_PREFIX}-missing-ssm-parameter -c test:ssm-parameter-name=${parameterName}" <<HERE
[Error at /${STACK_NAME_PREFIX}-missing-ssm-parameter] SSM parameter not available in account ${account}, region ${region}: /does/not/exist
[Error at /${STACK_NAME_PREFIX}-missing-ssm-parameter] SSM parameter not available in account ${TEST_ACCOUNT}, region ${TEST_REGION}: /does/not/exist
Found errors
HERE

Expand Down