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(doc): update the contributing guide with more details #25321

Merged
merged 10 commits into from
Apr 27, 2023

Conversation

pahud
Copy link
Contributor

@pahud pahud commented Apr 26, 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

@gitpod-io
Copy link

gitpod-io bot commented Apr 26, 2023

@aws-cdk-automation aws-cdk-automation requested a review from a team April 26, 2023 16:22
@github-actions github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 labels Apr 26, 2023
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Apr 26, 2023
@pahud pahud marked this pull request as ready for review April 26, 2023 16:30
CONTRIBUTING.md Outdated
```

This allows you to iterate your development and ensure a minimal sample app with your hack can
successfully deploy as you expect. Do not commit `sample.ts` and `sample.js` to your PR branch.
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, this is really important... Maybe call it out more?

@MrArnoldPalmer
Copy link
Contributor

MrArnoldPalmer commented Apr 26, 2023

@pahud I think instead of encouraging people to write sample apps they need to deploy for testing, they should just write it as an integration test and if they want to deploy the stack without deleting it after, they can run yarn integ integ.my-test.js --no-clean. This way they don't have to worry about not committing the file and there is less potential for using a globally installed CLI vs the locally built one etc.

@pahud
Copy link
Contributor Author

pahud commented Apr 26, 2023

@MrArnoldPalmer

Agree. yarn integ is very helpful for sample app testing but you lose the benefit of cdk diff to compare difference between deployments and also cdk watch as well as --hotswap for hotswappable resources. You will not be able to pass context variables to test different behaviors with cdk deploy -c or cdk diff -c.

And, to save my time on repeated deployment, I tend to deploy resources in my existing vpc like:

const cluster = new eks.Cluster(stack, 'Cluster', {
  vpc: ec2.Vpc.fromLookup(stack, 'Vpc', { isDefault: true }),
  ...getClusterVersionConfig(stack),
  defaultCapacity: 0,
});

But we can't commit this as part of our integ testing. So I think having a standalone sample ts that allows you to interact with just like a common CDK app is still very helpful and productive. But this is just my personal experience. I'll see if I can capture the advantages from both.

Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer left a comment

Choose a reason for hiding this comment

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

Just some language suggestions. I'm still not a fan of the sample app suggestion for most things because it feels unwieldy, but this is a personal preference for my own workflow I suppose. Everything that is accomplished by a test app like this can be accomplished by an integration test in a repeatable way.

I think for now this is a good guide for users to do a deploy as easily as possible if they want to use things like cdk diff and hotswap and points to some features we should add to integ-runner so I'll look into that separately.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated
Comment on lines 385 to 388
This allows you to iterate your development and ensure a minimal sample app successfully deploy as you expect.
You have the freedom to interact with it just as a common CDK app such as viewing differences with `npx cdk diff`
or pass context variables with `npx cdk deploy -c`. You can rapidly iterate your testing with repeated deployments
by importing existing resource such as existing VPC. This can save a lot of time and help you focus on the core changes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This allows you to iterate your development and ensure a minimal sample app successfully deploy as you expect.
You have the freedom to interact with it just as a common CDK app such as viewing differences with `npx cdk diff`
or pass context variables with `npx cdk deploy -c`. You can rapidly iterate your testing with repeated deployments
by importing existing resource such as existing VPC. This can save a lot of time and help you focus on the core changes.
This allows you to iterate your development and ensures a minimal sample app successfully deploys as expected.
You have the freedom to interact with it just as a common CDK app such as viewing differences with `npx cdk diff`
or pass context variables with `npx cdk deploy -c`. You can rapidly iterate your testing with repeated deployments
by importing existing resource such as an existing VPC. This can save a lot of time and help you focus on the core changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

CONTRIBUTING.md Outdated Show resolved Hide resolved
pahud and others added 3 commits April 26, 2023 18:25
Co-authored-by: Mitchell Valine <mitchellvaline@gmail.com>
Co-authored-by: Mitchell Valine <mitchellvaline@gmail.com>
Co-authored-by: Mitchell Valine <mitchellvaline@gmail.com>
@mergify
Copy link
Contributor

mergify bot commented Apr 26, 2023

Thank you for contributing! Your pull request will be updated from main 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: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 0c7c630
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Apr 27, 2023

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

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. 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 this pull request may close these issues.

core: refresh the contributing guide
4 participants