-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat: Validate PR description Changelogs on Eigen #183
Closed
Closed
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
cd435ee
run only on eigen
MounirDhahri 117ff18
run only on open PRs
MounirDhahri 9addea7
warns the author when the PR body is invalid
MounirDhahri ce10094
warns the author when no changelog changes were detected
MounirDhahri 38fd98d
return the list of changes depending on the PR changelog
MounirDhahri 8d7f387
link peril related RFC
MounirDhahri 24325bb
enable test only for eigen prs with #run_new_changelog_check
MounirDhahri 66d05d5
fix merge conflicts
MounirDhahri File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,10 @@ | ||
import { danger, warn, fail, GitHubCommit, markdown } from "danger" | ||
|
||
import yarn from "danger-plugin-yarn" | ||
import { ParseResult } from "./helpers/changelog/changelog-types" | ||
// @ts-ignore | ||
import { parsePRDescription } from "./helpers/changelog/parsePRDescription.js" | ||
// @ts-ignore | ||
import { changelogTemplateSections } from "./helpers/changelog/generateChangelogSectionTemplate.js" | ||
Comment on lines
+3
to
+7
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 think it might be better if I just create a new repo with these files to consume on both Eigen and here |
||
|
||
// "Highlight package dependencies on Node projects" | ||
const rfc1 = async () => { | ||
|
@@ -219,6 +223,62 @@ export const deploySummary = async () => { | |
return markdown(message) | ||
} | ||
|
||
// Require changelog on Eigen PRs to match to be valid | ||
// See Eigen RFC: https://github.com/artsy/eigen/issues/4499 | ||
// Peril related RFC: https://github.com/artsy/peril-settings/issues/182 | ||
export const rfc179 = () => { | ||
const pr = danger.github.pr | ||
const repoName = pr.base.repo.name | ||
|
||
if (repoName !== "eigen") { | ||
console.log("Skipping this check because the active repo is not Eigen") | ||
return | ||
} | ||
|
||
const isOpen = pr.state === "open" | ||
if (!isOpen) { | ||
console.log("Skipping this check because the PR is not open") | ||
return | ||
} | ||
|
||
const content = pr.body | ||
|
||
const res = parsePRDescription(content) as ParseResult | ||
|
||
// TODO: Delete this once we finish the new changelog work | ||
if (!content.includes("#run_new_changelog_check")) { | ||
return | ||
} | ||
|
||
if (res.type === "error") { | ||
console.log("Something went wrong while parsing the PR description") | ||
warn("❌ **An error occurred while validating your changelog, please make sure you provided a valid changelog.**") | ||
return | ||
} | ||
|
||
if (res.type === "no_changes") { | ||
console.log("PR has no changes") | ||
warn("✅ **No changelog changes**") | ||
return | ||
} | ||
|
||
// At this point, the PR description changelog changes are valid | ||
// and res contains a list of the changes | ||
console.log("PR Changelog is valid") | ||
|
||
const { type, ...changedSections } = res | ||
const message = | ||
"### This PR contains the following changes:\n" + | ||
Object.entries(changedSections) | ||
.map(([section, sectionValue]) => { | ||
return `\n- ${changelogTemplateSections[section]} (${sectionValue})` | ||
}) | ||
.join("") | ||
|
||
console.log(message) | ||
return markdown(message) | ||
} | ||
|
||
// Nudge PR authors to use semantic commit formatting | ||
// https://github.com/artsy/README/issues/327 | ||
export const rfc327 = () => { | ||
|
@@ -243,6 +303,7 @@ export default async () => { | |
await rfc13() | ||
await rfc16() | ||
await rfc177() | ||
rfc179() | ||
rfc327() | ||
await deploySummary() | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
export type ParseResult = | ||
| { type: "error" } // for when no changelog-related stuff is found | ||
| { type: "no_changes" } // for when we add #nochangelog hashtag | ||
| { | ||
type: "changes" | ||
crossPlatformUserFacingChanges: string[] | ||
iOSUserFacingChanges: string[] | ||
androidUserFacingChanges: string[] | ||
devChanges: string[] | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
// @ts-check | ||
|
||
const changelogTemplateSections = { | ||
crossPlatformUserFacingChanges: "Cross-platform user-facing changes", | ||
iOSUserFacingChanges: "iOS user-facing changes", | ||
androidUserFacingChanges: "Android user-facing changes", | ||
devChanges: "Dev changes", | ||
} | ||
|
||
module.exports.changelogTemplateSections = changelogTemplateSections | ||
|
||
module.exports.generateChangelogSectionTemplate = () => { | ||
return `### Changelog updates | ||
|
||
<!-- 📝 Please fill out at least one of these sections. --> | ||
<!-- ⓘ 'User-facing' changes will be published as release notes. --> | ||
<!-- ⌫ Feel free to remove sections that don't apply. --> | ||
<!-- • Write a markdown list or just a single paragraph, but stick to plain text. --> | ||
<!-- 🤷♂️ Replace this entire block with the hashtag \`#nochangelog\` to avoid updating the changelog. --> | ||
${Object.entries(changelogTemplateSections) | ||
.map(([, title]) => "#### " + title) | ||
.join("\n-\n")} | ||
- | ||
|
||
<!-- end_changelog_updates --> | ||
` | ||
} | ||
|
||
const fs = require("fs") | ||
const prettier = require("prettier") | ||
|
||
/** | ||
* @param {string} filePath | ||
*/ | ||
module.exports.updateChangelogSectionTemplate = (filePath) => { | ||
let fileContents = fs.readFileSync(filePath, "utf8").toString() | ||
|
||
const regex = /### Changelog updates[\S\s]+end_changelog_updates.*/g | ||
if (!fileContents.match(regex)) { | ||
console.error("Can't find 'Changelog updates' section in pull request template", filePath) | ||
process.exit(1) | ||
} | ||
|
||
fileContents = fileContents.replace(regex, this.generateChangelogSectionTemplate()) | ||
fileContents = prettier.format(fileContents, { parser: "markdown" }) | ||
|
||
fs.writeFileSync(filePath, fileContents, "utf8") | ||
} | ||
|
||
// @ts-ignore | ||
if (require.main === module) { | ||
this.updateChangelogSectionTemplate("./docs/pull_request_template.md") | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,191 @@ | ||
import { ParseResult as PRDescriptionParseResult } from "./changelog-types" | ||
// @ts-ignore | ||
import { parsePRDescription } from "./parsePRDescription" | ||
|
||
const ERROR: PRDescriptionParseResult = { type: "error" } | ||
const NO_CHANGES: PRDescriptionParseResult = { type: "no_changes" } | ||
|
||
describe("parsePRDescription", () => { | ||
it("returns error for empty PR description", () => { | ||
expect(parsePRDescription("")).toEqual(ERROR) | ||
}) | ||
|
||
it("returns error for PR description that does not contain any changelog info", () => { | ||
expect( | ||
parsePRDescription(` | ||
# Description | ||
|
||
This pull request adds some stuff to the thing so that it can blah. | ||
`) | ||
).toEqual(ERROR) | ||
}) | ||
|
||
it("returns no_changes when the user includes '#nochangelog' hashtag", () => { | ||
expect( | ||
parsePRDescription(` | ||
# Description | ||
|
||
This pull request adds some stuff to the thing so that it can blah. | ||
|
||
#nochangelog | ||
`) | ||
).toEqual(NO_CHANGES) | ||
}) | ||
|
||
it("returns error when no changes have been declared and #nochangelog has not been included", () => { | ||
expect( | ||
parsePRDescription(` | ||
# Description | ||
|
||
This pull request adds some stuff to the thing so that it can blah. | ||
### Changelog updates | ||
|
||
<!-- 📝 Please fill out at least one of these sections. --> | ||
<!-- ⓘ 'User-facing' changes will be published as release notes. --> | ||
<!-- ⌫ Feel free to remove sections that don't apply. --> | ||
<!-- • Write a markdown list or just a single paragraph, but stick to plain text. --> | ||
<!-- 🤷♂️ Replace this entire block with the hashtag \`#nochangelog\` to avoid updating the changelog. --> | ||
|
||
#### Cross-platform user-facing changes | ||
- | ||
#### iOS user-facing changes | ||
- | ||
#### Android user-facing changes | ||
- | ||
#### Dev changes | ||
- | ||
|
||
### Other stuff | ||
`) | ||
).toEqual(ERROR) | ||
}) | ||
|
||
it("returns any changes specified by the PR author", () => { | ||
expect( | ||
parsePRDescription(` | ||
# Description | ||
|
||
This pull request adds some stuff to the thing so that it can blah. | ||
### Changelog updates | ||
|
||
#### Cross-platform user-facing changes | ||
- Added a new button | ||
for the checkout flow | ||
- Fixed modal close button | ||
#### iOS user-facing changes | ||
- Fixed input focus styles | ||
#### Android user-facing changes | ||
Updated splash screen color | ||
#### Dev changes | ||
- Improved changelog tooling | ||
- Upgraded lodash | ||
|
||
### Other stuff | ||
|
||
blah | ||
`) | ||
).toMatchInlineSnapshot(` | ||
Object { | ||
"androidUserFacingChanges": Array [ | ||
"Updated splash screen color", | ||
], | ||
"crossPlatformUserFacingChanges": Array [ | ||
"Added a new button | ||
for the checkout flow", | ||
"Fixed modal close button", | ||
], | ||
"devChanges": Array [ | ||
"Improved changelog tooling", | ||
"Upgraded lodash", | ||
], | ||
"iOSUserFacingChanges": Array [ | ||
"Fixed input focus styles", | ||
], | ||
"type": "changes", | ||
} | ||
`) | ||
}) | ||
|
||
it("allows sections of the changelog to be deleted", () => { | ||
expect( | ||
parsePRDescription(` | ||
### Description | ||
blah blah | ||
|
||
### Changelog updates | ||
|
||
#### iOS user-facing changes | ||
- Fixed input focus styles | ||
|
||
### Other stuff | ||
|
||
blah | ||
`) | ||
).toMatchInlineSnapshot(` | ||
Object { | ||
"androidUserFacingChanges": Array [], | ||
"crossPlatformUserFacingChanges": Array [], | ||
"devChanges": Array [], | ||
"iOSUserFacingChanges": Array [ | ||
"Fixed input focus styles", | ||
], | ||
"type": "changes", | ||
} | ||
`) | ||
}) | ||
|
||
it("supports single-level markdown lists", () => { | ||
expect( | ||
parsePRDescription(` | ||
### Changelog updates | ||
|
||
#### iOS user-facing changes | ||
- Fixed input focus styles | ||
- Added things | ||
- Removed things | ||
- Updated things | ||
`) | ||
).toMatchInlineSnapshot(` | ||
Object { | ||
"androidUserFacingChanges": Array [], | ||
"crossPlatformUserFacingChanges": Array [], | ||
"devChanges": Array [], | ||
"iOSUserFacingChanges": Array [ | ||
"Fixed input focus styles", | ||
"Added things", | ||
"Removed things", | ||
"Updated things", | ||
], | ||
"type": "changes", | ||
} | ||
`) | ||
}) | ||
|
||
it("supports paragraphs of text", () => { | ||
expect( | ||
parsePRDescription(` | ||
### Changelog updates | ||
|
||
#### iOS user-facing changes | ||
made the modals close a bit faster | ||
by updating the transition gradient | ||
|
||
uses a trampoline to avoid the problem | ||
where the click didn't work | ||
`) | ||
).toMatchInlineSnapshot(` | ||
Object { | ||
"androidUserFacingChanges": Array [], | ||
"crossPlatformUserFacingChanges": Array [], | ||
"devChanges": Array [], | ||
"iOSUserFacingChanges": Array [ | ||
"made the modals close a bit faster | ||
by updating the transition gradient", | ||
"uses a trampoline to avoid the problem | ||
where the click didn't work", | ||
], | ||
"type": "changes", | ||
} | ||
`) | ||
}) | ||
}) | ||
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. Great tests 🙌 |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
why do we need ts-ignore here?
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.
typescript had issues importing javascript files. Maybe I can try to update the typescript config file to fix that as well