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

core: refresh the contributing guide #25196

Closed
1 of 2 tasks
pahud opened this issue Apr 19, 2023 · 2 comments · Fixed by #25321
Closed
1 of 2 tasks

core: refresh the contributing guide #25196

pahud opened this issue Apr 19, 2023 · 2 comments · Fixed by #25321
Assignees
Labels
@aws-cdk/core Related to core CDK functionality effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1

Comments

@pahud
Copy link
Contributor

pahud commented Apr 19, 2023

Describe the feature

The contributing guide has been missing a lot of information since the repo remodel and is getting very difficult for the community to contribute the pull requests. I think we should update the contributing guide to clarify some important steps including:

  1. How to build the core package(aws-cdk-lib).
  2. How to run yarn watch when we start our hacking on individual module(such as aws-eks).
  3. After we modify the source, how to deploy a simple app with our fix in our AWS account to validate it.
  4. How to run all unit tests on a specific module(e.g. aws-eks).
  5. How to run a specific unit test on a specific module(e.g. aws-eks).
  6. How to run all integ tests on specific module(e.g. aws-eks).
  7. How to run a specific integ test and update the snapshots on failed.
  8. How to run awslint or any other linting tools before submitting the PR.
  9. How to avoid common errors such as JavaScript heap out of memory error described in (aws-cdk-lib): Reached heap limit Allocation failed - JavaScript heap out of memory #25092

Use Case

After the repo remodel, contributors now feel confused and the contributing guide is out-of-date.

Proposed Solution

Update the contributing guide with relevant steps and guidance.

Other Information

This is my personal note for a recent PR to aws-eks but I expect a formal guidance.

# in aws-cdk root directory
$ export NODE_OPTIONS=--max-old-space-size=8192
$ yarn install
$ cd packages/aws-cdk-lib
$ ../../scripts/buildup 
# or
$ npx lerna run build --scope=aws-cdk-lib
# now run `yarn watch` in `aws-cdk-lib`
$ yarn watch
# cd to individual module directory and start your hack
$ cd aws-eks
# now, run the unit tests for this individual module
$ yarn test aws-eks
# now validate the existing integ tests in framework-integ
$ cd <cdk_root>/packages/@aws-cdk-testing/framework-integ
# we need buildup first
$ ../../../scripts/buildup 
# now let's run all existing integ tests for aws-eks
$ yarn integ --directory test/aws-eks/test/
# If any single integ test fails, run with `--update-on-failed`. This triggers a real deployment and snapshot update.
$ yarn integ --update-on-failed test/aws-eks/test/integ.xxx.js
# test again without `--update-on-failed`
$ yarn integ test/aws-eks/test/integ.xxx.js
# when we fix all individual integ tests, run against the directory again
$ yarn integ --directory test/aws-eks/test/
# now you should pass all unit tests and integ tests

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

v2.72.1

Environment details (OS name and version, etc.)

mac os x

@pahud pahud added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Apr 19, 2023
@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label Apr 19, 2023
@pahud pahud removed the needs-triage This issue or PR still needs to be triaged. label Apr 19, 2023
@peterwoodworth peterwoodworth added p1 effort/small Small work item – less than a day of effort labels Apr 21, 2023
@mattiamatrix
Copy link
Contributor

npx lerna run build --scope=aws-cdk-lib is currently failing for me.

...
100_sam/998_lift_transform_patch.json: AWS::Serverless::LayerVersion: Lift transform version to each resource
100_sam/998_lift_transform_patch.json: AWS::Serverless::SimpleTable: Lift transform version to each resource
100_sam/998_lift_transform_patch.json: AWS::Serverless::StateMachine: Lift transform version to each resource
100_sam/999_cleanup_patch.json: Remove properties that conflict with the CFN spec
spec-source/specification/100_sam
TypeError: Cannot use 'in' operator to search for 'Properties' in undefined
    at Object.isRecordType (/Users/***/Code/GitHub/***/aws-cdk/packages/@aws-cdk/cfnspec/lib/schema/specification.ts:55:22)
    at replaceIncompleteTypes (/Users/***/Code/GitHub/***/aws-cdk/packages/@aws-cdk/cfnspec/build-tools/massage-spec.ts:20:17)
    at massageSpec (/Users/***/Code/GitHub/***/aws-cdk/packages/@aws-cdk/cfnspec/build-tools/massage-spec.ts:6:3)
    at generateResourceSpecification (/Users/***/Code/GitHub/***/aws-cdk/packages/@aws-cdk/cfnspec/build-tools/build.ts:42:14)
    at async main (/Users/***/Code/GitHub/***/aws-cdk/packages/@aws-cdk/cfnspec/build-tools/build.ts:28:5)
error Command failed with exit code 255.

@mergify mergify bot closed this as completed in #25321 Apr 27, 2023
mergify bot pushed a commit that referenced this issue Apr 27, 2023
This PR explains how to `Verify your fix by deployment` before you submit a pull request.

When contributors complete their hack on any modules under `aws-cdk-lib`, it's unclear to them how to verify and ensure their fix can successfully synthesize and deploy in a real AWS environment. This PR explains how to do that with more details including:

1. How to write a minimal cdk App with your fix to verify it in your AWS account
2. How to run all unit tests against your hack
3. How to run a single unit test against your hack
4. How to run all integ tests against your hack
5. How to run a single integ test against your hack

With the additional content, contributors will be able to iterate their development easily and ensure their hack can successfully deploy in their own AWS accounts as expected before they submit their PRs.

Closes #25196

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/core Related to core CDK functionality effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants