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(docs): ensure code snippets in aws-lambda README compile #14753

Merged
merged 5 commits into from
Jun 29, 2021

Conversation

BenChaimberg
Copy link
Contributor

Update README in aws-lambda package to pass strict Rosetta compilation, ie., all the Typescript snippets in the README compile successfully. Also follows convention for unscoped usage of all local names (anything from the aws-lambda package) and scoped usage of all other names.

In particular:

  • removes many lambda. qualifiers
  • creates a "default" fixture with a stack already created (accessible as this) and lots of names from the package already supported
  • creates a "function" fixture that includes an already-built function (accessible as fn)

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

@gitpod-io
Copy link

gitpod-io bot commented May 18, 2021

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label May 18, 2021
@BenChaimberg BenChaimberg requested a review from nija-at May 20, 2021 19:54
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Not super familiar with the options available for rosetta. Is there a way to fail build if a fixture and/or snippet fails to compile?

If not, this is going to be broken (again) as soon you look the other way.

@BenChaimberg
Copy link
Contributor Author

Yes, there is the --strict flag that will cause rosetta to exit unsuccessfully if a snippet fails to compile. It is also available to be configured via the package.json. However, since we previously decided not to compile snippets during build, this would only cause a failure during packing/release, not during PR build. We could revisit this decision since, by my understanding, rosetta uses the already-existing jsii manifest to discover code snippets and doesn't re-compile. Alternatively, we could add it to the linting step, which wouldn't affect the watch command.

@nija-at
Copy link
Contributor

nija-at commented May 26, 2021

Awesome diving deep into some previously made decisions to figure this out!

I would suggest that we do the suggestion from this comment you referred above:

make it a separate npm script for developers to run, OR only do this conditionally under an environment variable

Add an npm script, say executed via yarn rosetta, for each of our packages that does the build and an additional environment variable that is enabled for PR build.
Adding it to PR build is important; we want contributors (from the team and outside) to fix the code snippets as part of their PR. Failing at the 'pack' step in our build pipeline makes it the oncall's problem.

The third missing piece would be to use a package.json entry which enables (or disables) rosetta compilation for packages as we start fixing existing snippets.

Doing these first (as a separate PR) before fixing up the actual code snippets will guarantee that your work here continues to be used, and doesn't regress in the next change to these files.

cc @rix0rrr


Adding an npm script to all of our 200+ packages in not as hard as it sounds. If you didn't know already, we have pkglint for that very use. Modify the rule once and run yarn pkglint from the root of the repo to fix all relevant packages.

Happy to chat about how I would approach other things I've mentioned above.

@BenChaimberg
Copy link
Contributor Author

This sounds like a good plan! Most of this is already in place and can leverage the "strict" metadata entry in the package.json, as described above. However, there seem to be some bugs in rosetta that cause this entry to be ignored. Opened aws/jsii#2861 which, upon fix, should unblock this work.

@mergify
Copy link
Contributor

mergify bot commented Jun 23, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify
Copy link
Contributor

mergify bot commented Jun 29, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 1f5360e into master Jun 29, 2021
@mergify mergify bot deleted the chaimber/lambda_docs branch June 29, 2021 18:44
@mergify
Copy link
Contributor

mergify bot commented Jun 29, 2021

Thank you for contributing! Your pull request will be updated from master 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: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: d5a55dc
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
…4753)

Update README in aws-lambda package to pass strict Rosetta compilation, ie., all the Typescript snippets in the README compile successfully. Also follows convention for unscoped usage of all local names (anything from the aws-lambda package) and scoped usage of all other names.

In particular:
- removes many `lambda.` qualifiers
- creates a "default" fixture with a stack already created (accessible as `this`) and lots of names from the package already supported
- creates a "function" fixture that includes an already-built function (accessible as `fn`)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants