diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 99093538f6b52..caa7b80a94a8c 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,37 +1,10 @@ -## Description - - -## Commit Message - -{*replace-with-pr-title*} (#{*replace-with-pr-number*}) - - -{replace-with-extended-commit-message} - - - - - - - - - - -## End Commit Message ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* - diff --git a/.github/actions/prlinter/index.js b/.github/actions/prlinter/index.js index b15aad8a9ca1a..e36cc7aa9ffba 100644 --- a/.github/actions/prlinter/index.js +++ b/.github/actions/prlinter/index.js @@ -3,8 +3,7 @@ const github = require('@actions/github'); const linter = require('prlint') const checks = { - "MANDATORY_CHANGES": linter.mandatoryChanges, - "COMMIT_MESSAGE": linter.commitMessage + "MANDATORY_CHANGES": linter.mandatoryChanges } async function run() { @@ -21,7 +20,7 @@ async function run() { } await check(number); - + } catch (error) { core.setFailed(error.message); diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1c34721284646..20c5277e90b23 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -11,8 +11,9 @@ and let us know if it's not up-to-date (even better, submit a PR with your corr - [Step 1: Open Issue](#step-1-open-issue) - [Step 2: Design (optional)](#step-2-design-optional) - [Step 3: Work your Magic](#step-3-work-your-magic) - - [Step 4: Pull Request](#step-4-pull-request) - - [Step 5: Merge](#step-5-merge) + - [Step 4: Commit](#step-4-commit) + - [Step 5: Pull Request](#step-5-pull-request) + - [Step 6: Merge](#step-6-merge) - [Tools](#tools) - [Main build scripts](#main-build-scripts) - [Partial build tools](#partial-build-tools) @@ -52,7 +53,7 @@ For day-to-day development and normal contributions, the following SDKs and tool - [.NET Core SDK 3.0](https://www.microsoft.com/net/download) - [Python 3.6.5](https://www.python.org/downloads/release/python-365/) - [Ruby 2.5.1](https://www.ruby-lang.org/en/news/2018/03/28/ruby-2-5-1-released/) - + The basic commands to get the repository cloned and built locally follow: ```console @@ -141,7 +142,7 @@ Integration tests perform a few functions in the CDK code base - 3. (Optionally) Acts as a way to validate that constructs set up the CloudFormation resources as expected. A successful CloudFormation deployment does not mean that the resources are set up correctly. -If you are working on a new feature that is using previously unused CloudFormation resource types, or involves +If you are working on a new feature that is using previously unused CloudFormation resource types, or involves configuring resource types across services, you need to write integration tests that use these resource types or features. @@ -161,37 +162,48 @@ Examples: * [integ.destinations.ts](https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-lambda-destinations/test/integ.destinations.ts#L7) * [integ.token-authorizer.ts](https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-apigateway/test/authorizers/integ.token-authorizer.ts#L6) -### Step 4: Pull Request +### Step 4: Commit -* Push to a GitHub fork or to a branch (naming convention: `/`) -* Submit a Pull Request on GitHub and assign the PR for a review to the "aws/aws-cdk-team" team. The title and description will be used to format the commit message when its merged to master. This in turn, will translate to CHANGELOG entries. It is therefore important we be consistent and informative. Here is an example PR you should use as a reference: https://github.com/aws/aws-cdk/pull/6553. +Create a commit with the proposed change changes: - ### Title +* Commit title and message (and PR title and description) must adhere to [conventionalcommits](https://www.conventionalcommits.org). + * The title must begin with `feat(module): title`, `fix(module): title`, `refactor(module): title` or + `chore(module): title`. + * Title should be lowercase. + * No period at the end of the title. - * Must adhere to [conventionalcommits](https://www.conventionalcommits.org). - * The title must begin with one of: - - `feat(module): title` - - `fix(module): title` - - `refactor(module): title` - - `chore(module): title` - * Should be lowercase. - * No period at the end. +* Commit message should describe _motivation_. Think about your code reviewers and what information they need in + order to understand what you did. If it's a big commit (hopefully not), try to provide some good entry points so + it will be easier to follow. +* Commit message should indicate which issues are fixed: `fixes #` or `closes #`. - ### Description +* Shout out to collaborators. - * Simply follow the PR template carefully. +* If not obvious (i.e. from unit tests), describe how you verified that your change works. +* If this commit includes breaking changes, they must be listed at the end in the following format (notice how multiple breaking changes should be formatted): +``` +BREAKING CHANGE: Description of what broke and how to achieve this behavior now +* **module-name:** Another breaking change +* **module-name:** Yet another breaking change +``` + +### Step 5: Pull Request + +* Push to a GitHub fork or to a branch (naming convention: `/`) +* Submit a Pull Requests on GitHub and assign the PR for a review to the "awslabs/aws-cdk" team. * Please follow the PR checklist written below. We trust our contributors to self-check, and this helps that process! * Discuss review comments and iterate until you get at least one “Approve”. When iterating, push new commits to the same branch. Usually all these are going to be squashed when you merge to master. The commit messages should be hints for you when you finalize your merge commit message. -* Make sure to update the PR title/description if things change. +* Make sure to update the PR title/description if things change. The PR title/description are going to be used as the + commit title/message and will appear in the CHANGELOG, so maintain them all the way throughout the process. -### Step 5: Merge +### Step 6: Merge * Make sure your PR builds successfully (we have CodeBuild setup to automatically build all PRs) * Once approved and tested, a maintainer will squash-merge to master and will use your PR title/description as the diff --git a/tools/prlint/index.js b/tools/prlint/index.js index b5b0d55c3ba6e..7e4212d1ef9ea 100755 --- a/tools/prlint/index.js +++ b/tools/prlint/index.js @@ -18,7 +18,7 @@ function createGitHubClient() { } else { console.log("Creating un-authenticated GitHub Client") } - + return new GitHub({'token': token}); } @@ -67,10 +67,10 @@ async function mandatoryChanges(number) { } const gh = createGitHubClient(); - + const issues = gh.getIssues(OWNER, REPO); const repo = gh.getRepo(OWNER, REPO); - + console.log(`⌛ Fetching PR number ${number}`) const issue = (await issues.getIssue(number)).data; @@ -84,69 +84,7 @@ async function mandatoryChanges(number) { fixContainsTest(issue, files); console.log("✅ Success") - -} - -async function commitMessage(number) { - - function validate() { - - // this is the commit message mergify will use. - // see https://doc.mergify.io/actions.html#commit-message-and-squash-method. - const commitMessageSection = issue.body.match(/## Commit Message([\s|\S]*)## End Commit Message/); - - if (!commitMessageSection || commitMessageSection.length !== 2) { - throw new LinterError("Your PR description doesn't specify the commit" - + " message properly. See for details.") - } - - const commitMessage = commitMessageSection[1].trim(); - - const paragraphs = commitMessage.split(/\r\n\r\n|\n\n/); - const title = paragraphs[0]; - const expectedCommitTitle = `${issue.title} (#${number})` - - if (title !== expectedCommitTitle) { - throw new LinterError("First paragraph of '## Commit Message' section" - + ` must be: '${expectedCommitTitle}'`) - } - - for (i in paragraphs) { - if (i != paragraphs.length - 1 && paragraphs[i].startsWith("BREAKING CHANGE:")) { - throw new LinterError("'BREAKING CHANGE:' must be specified as the last paragraph"); - } - } - } - - if (!number) { - throw new Error('Must provide a PR number') - } - - const gh = createGitHubClient(); - - const issues = gh.getIssues(OWNER, REPO); - - console.log(`⌛ Fetching PR number ${number}`) - const issue = (await issues.getIssue(number)).data; - - const noSquash = issue.labels.some(function (l) { - return l.name.includes("no-squash"); - }); - - if (issue.user.login === "dependabot[bot]" || issue.user.login === "dependabot-preview[bot]") { - // dependabot PR's are ok even without following this convention because they only contain - // a single commit in conventional commit form. - console.log("⏭️ Validation skipped because its a dependabot PR"); - } else if (noSquash) { - // if the PR isn't merged as a squash commit, all this validation is irrelevant. - // this is the case for our automatic PR's to the 'release' branch. - console.log("⏭️ Validation skipped because the PR is labeled with 'no-squash'"); - } else { - console.log("⌛ Validating..."); - validate(); - } - - console.log("✅ Success") + } // we don't use the 'export' prefix because github actions @@ -154,7 +92,6 @@ async function commitMessage(number) { // TODO need to verify this. module.exports.mandatoryChanges = mandatoryChanges module.exports.LinterError = LinterError -module.exports.commitMessage = commitMessage require('make-runnable/custom')({ printOutputFrame: false