chore: make examples in `aws-ec2` package compilable #5011

rix0rrr · 2019-11-13T14:17:14Z

Commit Message

chore: make examples in aws-ec2 package compilable

Using the new "rosetta" sample compiler from aws/jsii#925, introduce
fixtures and fix up sample code to make all examples in the aws-ec2
package compile.

This serves as a demonstration of how to set up fixtures and how
to write the examples.

Need to reverse the order between @example and @attribute; because
of a bug in the TypeScript compiler, it will think a new tag starts if
a @ comes up in the example. This means that we cannot type
import s3 = require('@aws-cdk/aws-s3'); in examples (it would parse @aws = -cdk/aws-s3');.

To fix that, we absorb non-recognized tags that follow an @example
back into the example body, but since @attribute is non-recognized,
we'd absorb that as well. The solution is to have @example as the last tag.

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

Using the new "rosetta" sample compiler from aws/jsii#925, introduce fixtures and fix up sample code to make all examples in the `aws-ec2` package compile. This serves as a demonstration of how to set up fixtures and how to write the examples.

mergify · 2019-11-13T14:17:55Z

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-11-13T14:17:56Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

rix0rrr · 2019-11-13T14:20:04Z

And again, this obviously will not compile before we have released and merged the requisite JSII tooling.

@example

If we don't do this, the '@example' will absorb the non-standard '@Attribute' tag.

aws-cdk-automation · 2019-11-13T14:33:58Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

CONTRIBUTING.md

nija-at · 2019-11-14T11:27:01Z

CONTRIBUTING.md

+```
+
+While processing the examples, the tool will look for a file called
+`rosetta/with-bucket.ts-fixture` in the package directory. This file will be


Perhaps a bit late now but would be better to have used the extension format .ts.fixture in line with how template files are usually named like .html.erb, .template.json, etc.

That should be .fixture.ts then. I recall not doing that because TSC will want to compile it then, something like that.

Hence the suggestion to go .ts.fixture.

Rationale: The file might not be strictly typescript (or atleast not a complete typescript app). When the fixture is resolved (with the example code), it becomes correct typescript (or fully valid).

Non-blocking.

nija-at · 2019-11-14T11:37:04Z

CONTRIBUTING.md

+treated as a regular TypeScript source file, but it must also contain the text
+`/// here`, at which point the example will be inserted. The complete file must
+compile properly.
+
+Before the `/// here` marker, the fixture should import the necessary packages
+and initialize the required variables.


I got a bit lost in the text here, until I saw the ec2 package.
Can we clean up the lang a bit and/or mention the examples in the ec2 packages earlier on?

CONTRIBUTING.md

nija-at · 2019-11-14T11:45:44Z

pack.sh

+# Compile examples with respect to "decdk" directory, as all packages will
+# be symlinked there so they can all be included.
+echo "Extracting code samples" >&2
+node --experimental-worker $(which jsii-rosetta) \
+  --compile \
+  --output samples.tabl.json \
+  --directory packages/decdk \
+  $(cat $TMPDIR/jsii.txt)


This should ideally be in the build step of each module so that I can see errors on invalid example code. If this happens only in the pack step, I won't know of this until a PR build.
Why not do this in the build step as done in compile samples?

Also, this is technically not 'packing'. If I were to speed up PR builds by removing packing, we would still want this to be present.

Why not do this in the build step as done in compile samples?

Because it's not hermetic. Samples will require everything to have been built and linked into decdk package.

nija-at · 2019-11-14T11:53:44Z

scripts/compile-samples

+
+        # Run rosetta against decdk dir, failing on compilation errors
+        node --experimental-worker $scriptdir/../node_modules/.bin/jsii-rosetta \
+            --directory $scriptdir/../packages/decdk \


Help me understand why we need to run this against decdk and can't be run against the current package's node_modules? Also, should decdk have been pre-built for this to work?

No just linked.

Because otherwise in aws-events package I cannot write samples that use aws-events-targets, for example (which I really want to do).

CONTRIBUTING.md

mergify · 2019-12-16T12:34:04Z

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

mergify · 2019-12-16T13:01:13Z

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

mergify · 2019-12-16T13:53:22Z

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

eladb · 2019-12-16T14:13:47Z

.gitignore

@@ -28,4 +28,4 @@ coverage/
 cdk.context.json
 .cdk.staging/
 cdk.out/
-
+*.tabl.json


do we want this in pkglint?

It's just going to be created here, not in any subpackage. So not really?

eladb · 2019-12-16T14:16:35Z

packages/@aws-cdk/aws-ec2/rosetta/with-vpc.ts-fixture

@@ -0,0 +1,13 @@
+// Fixture with packages imported and a VPC created


wouldn't it make more sense to use .fixture.ts extension so the IDE/github will know this is typescript?

Maybe, but the problem is the include paths won't resolve in the location where the files are stored so the IDE will start complaining and showing permanent errors in the Errors tab.

I've decided against that for now.

mergify · 2019-12-16T14:39:08Z

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

mergify · 2019-12-16T14:52:50Z

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

mergify · 2019-12-16T15:18:04Z

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 · 2020-01-03T16:07:45Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

Stale

mergify · 2020-01-06T09:29:07Z

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

aws-cdk-automation · 2020-01-06T10:04: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

mergify · 2020-01-06T10:04:33Z

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

rix0rrr requested review from RomainMuller, skinny85, eladb, nija-at and shivlaks November 13, 2019 14:17

rix0rrr self-assigned this Nov 13, 2019

mergify bot added the contribution/core This is a PR that came from AWS. label Nov 13, 2019

Merge remote-tracking branch 'origin/master' into huijbers/ec2-samples

34fc1d1

Reverse order between '@example' and '@Attribute'

1652578

If we don't do this, the '@example' will absorb the non-standard '@Attribute' tag.

nija-at previously requested changes Nov 14, 2019

View reviewed changes

eladb approved these changes Dec 16, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into huijbers/ec2-samples

27672b7

rix0rrr requested a review from nija-at January 3, 2020 15:38

Merge branch 'master' into huijbers/ec2-samples

b5bc491

mergify bot merged commit 7885025 into master Jan 6, 2020

mergify bot deleted the huijbers/ec2-samples branch January 6, 2020 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: make examples in `aws-ec2` package compilable #5011

chore: make examples in `aws-ec2` package compilable #5011

rix0rrr commented Nov 13, 2019 •

edited

Loading

mergify bot commented Nov 13, 2019

aws-cdk-automation commented Nov 13, 2019

rix0rrr commented Nov 13, 2019

aws-cdk-automation commented Nov 13, 2019

nija-at Nov 14, 2019

rix0rrr Nov 14, 2019

nija-at Nov 18, 2019

nija-at Nov 14, 2019

nija-at Nov 14, 2019

rix0rrr Nov 14, 2019

nija-at Nov 14, 2019

rix0rrr Nov 14, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

eladb Dec 16, 2019

rix0rrr Jan 3, 2020

eladb Dec 16, 2019

rix0rrr Jan 3, 2020

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

aws-cdk-automation commented Jan 3, 2020

mergify bot commented Jan 6, 2020

aws-cdk-automation commented Jan 6, 2020

mergify bot commented Jan 6, 2020

		@@ -0,0 +1,13 @@
		// Fixture with packages imported and a VPC created

chore: make examples in aws-ec2 package compilable #5011

chore: make examples in aws-ec2 package compilable #5011

Conversation

rix0rrr commented Nov 13, 2019 • edited Loading

Commit Message

mergify bot commented Nov 13, 2019

aws-cdk-automation commented Nov 13, 2019

AWS CodeBuild CI Report

rix0rrr commented Nov 13, 2019

aws-cdk-automation commented Nov 13, 2019

AWS CodeBuild CI Report

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

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

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

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

aws-cdk-automation commented Jan 3, 2020

AWS CodeBuild CI Report

mergify bot commented Jan 6, 2020

aws-cdk-automation commented Jan 6, 2020

AWS CodeBuild CI Report

mergify bot commented Jan 6, 2020

chore: make examples in `aws-ec2` package compilable #5011

chore: make examples in `aws-ec2` package compilable #5011

rix0rrr commented Nov 13, 2019 •

edited

Loading