-
Notifications
You must be signed in to change notification settings - Fork 4k
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: make examples in aws-ec2
package compilable
#5011
Changes from all commits
2b160ad
34fc1d1
1652578
27672b7
b5bc491
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,4 +28,4 @@ coverage/ | |
cdk.context.json | ||
.cdk.staging/ | ||
cdk.out/ | ||
|
||
*.tabl.json | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,7 @@ and let us know if it's not up-to-date (even better, submit a PR with your corr | |
- [Updating all Dependencies](#updating-all-dependencies) | ||
- [Running CLI integration tests](#running-cli-integration-tests) | ||
- [API Compatibility Checks](#api-compatibility-checks) | ||
- [Examples](#examples) | ||
- [Feature Flags](#feature-flags) | ||
- [Troubleshooting](#troubleshooting) | ||
- [Debugging](#debugging) | ||
|
@@ -527,6 +528,62 @@ this API we will not break anyone, because they weren't able to use it. The file | |
`allowed-breaking-changes.txt` in the root of the repo is an exclusion file that | ||
can be used in these cases. | ||
|
||
### Examples | ||
|
||
Examples typed in fenced code blocks (looking like `'''ts`, but then with backticks | ||
rix0rrr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
instead of regular quotes) will be automatically extrated, compiled and translated | ||
to other languages when the bindings are generated. | ||
|
||
To successfully do that, they must be compilable. The easiest way to do that is using | ||
a *fixture*, which looks like this: | ||
|
||
``` | ||
'''ts fixture=with-bucket | ||
bucket.addLifecycleTransition({ ... }); | ||
''' | ||
``` | ||
rix0rrr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps a bit late now but would be better to have used the extension format There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hence the suggestion to go 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. |
||
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. | ||
Comment on lines
+548
to
+553
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I got a bit lost in the text here, until I saw the ec2 package. |
||
|
||
If no fixture is specified, the fixture with the name | ||
`rosetta/default.ts-fixture` will be used if present. `nofixture` can be used to | ||
opt out of that behavior. | ||
rix0rrr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
In an `@example` block, which is unfenced, the first line of the example can | ||
contain three slashes to achieve the same effect: | ||
|
||
``` | ||
/** | ||
* @example | ||
* /// fixture=with-bucket | ||
* bucket.addLifecycleTransition({ ... }); | ||
*/ | ||
``` | ||
|
||
When including packages in your examples (even the package you're writing the | ||
examples for), use the full package name (e.g. `import s3 = | ||
require('@aws-cdk/aws-s3);`). The example will be compiled in an environment | ||
where all CDK packages are available using their public names. In this way, | ||
it's also possible to import packages that are not in the dependency set of | ||
the current package. | ||
|
||
For a practical example of how making sample code compilable works, see the | ||
`aws-ec2` package. | ||
|
||
Examples of all packages are extracted and compiled as part of the packaging | ||
step. If you are working on getting rid of example compilation errors of a | ||
single package, you can run `scripts/compile-samples` on the package by itself. | ||
rix0rrr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
For now, non-compiling examples will not yet block the build, but at some point | ||
in the future they will. | ||
|
||
### Feature Flags | ||
|
||
Sometimes we want to introduce new breaking behavior because we believe this is | ||
|
@@ -559,9 +616,9 @@ The pattern is simple: | |
5. Under `BREAKING CHANGES` in your commit message describe this new behavior: | ||
|
||
``` | ||
BREAKING CHANGE: template file names for new projects created through "cdk init" | ||
will use the template artifact ID instead of the physical stack name to enable | ||
multiple stacks to use the same name. This is enabled through the flag | ||
BREAKING CHANGE: template file names for new projects created through "cdk init" | ||
will use the template artifact ID instead of the physical stack name to enable | ||
multiple stacks to use the same name. This is enabled through the flag | ||
`@aws-cdk/core:enableStackNameDuplicates` in newly generated `cdk.json` files. | ||
``` | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,22 +25,31 @@ function lerna_scopes() { | |
done | ||
} | ||
|
||
echo "Packaging jsii modules" >&2 | ||
# 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) | ||
Comment on lines
+28
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Because it's not hermetic. Samples will require everything to have been built and linked into |
||
|
||
# Jsii packaging (all at once using jsii-pacmak) | ||
echo "Packaging jsii modules" >&2 | ||
jsii-pacmak \ | ||
--verbose \ | ||
--outdir $distdir/ \ | ||
--rosetta-tablet samples.tabl.json \ | ||
$(cat $TMPDIR/jsii.txt) | ||
|
||
# Non-jsii packaging, which means running 'package' in every individual | ||
# module and rsync'ing the result to the shared dist directory. | ||
# module | ||
echo "Packaging non-jsii modules" >&2 | ||
lerna run $(lerna_scopes $(cat $TMPDIR/nonjsii.txt)) --sort --concurrency=1 --stream package | ||
|
||
# Finally rsync all 'dist' directories together into a global 'dist' directory | ||
for dir in $(find packages -name dist | grep -v node_modules | grep -v run-wrappers); do | ||
echo "Merging ${dir} into ${distdir}" | ||
rsync -av $dir/ ${distdir}/ | ||
echo "Merging ${dir} into ${distdir}" >&2 | ||
rsync -a $dir/ ${distdir}/ | ||
done | ||
|
||
# Remove a JSII aggregate POM that may have snuk past | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want this in pkglint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just going to be created here, not in any subpackage. So not really?