From 0a1323fd796b67ef2f633c9f7d62020550c2caba Mon Sep 17 00:00:00 2001 From: Tomas Sebestik Date: Wed, 29 Nov 2023 15:42:50 +0100 Subject: [PATCH] feat(output-message): add output message and dynamic instructions --- action.yml | 22 +++ src/configParameters.ts | 31 ++-- src/dangerfile.ts | 4 + src/outputInstructions.ts | 103 +++++++++++++ src/ruleTargetBranch.ts | 6 +- tests/outputInstructions.test.ts | 249 +++++++++++++++++++++++++++++++ 6 files changed, 398 insertions(+), 17 deletions(-) create mode 100644 src/outputInstructions.ts create mode 100644 tests/outputInstructions.test.ts diff --git a/action.yml b/action.yml index 179344e..895daae 100644 --- a/action.yml +++ b/action.yml @@ -46,6 +46,23 @@ inputs: description: 'Enabled rule for check of PR target branch is default branch' required: false + instructions-enabled: + description: 'Enabled output instructions under issue table in the PR comment' + required: false + + instructions-contributions-guide-file: + description: 'Custom name of Contributions Guide file in repository' + required: false + + instructions-is-gitlab-mirror: + description: 'Enabled info about this is only Gitlab mirror' + required: false + + instructions-cla-link: + description: 'Link to project Contributor License Agreement' + required: false + + runs: using: 'docker' image: 'Dockerfile' @@ -67,3 +84,8 @@ runs: ENABLE_CHECK_PR_SOURCE_BRANCH_NAME: ${{ inputs.pr-source-branch-rule-enabled }} ENABLE_CHECK_PR_TARGET_BRANCH: ${{ inputs.pr-target-branch-rule-enabled }} + + ENABLE_OUTPUT_INSTRUCTIONS: ${{ inputs.instructions-enabled }} + CONTRIBUTING_GUIDE_FILE: ${{ inputs.instructions-contributions-guide-file }} + IS_GITLAB_MIRROR: ${{ inputs.instructions-is-gitlab-mirror}} + CLA_LINK: ${{ inputs.instructions-cla-link}} diff --git a/src/configParameters.ts b/src/configParameters.ts index be7862c..898a533 100644 --- a/src/configParameters.ts +++ b/src/configParameters.ts @@ -5,6 +5,12 @@ * If an environment variable for a parameter is not set, the system will use the default value from this object. */ const defaults = { + instructions: { + enabled: true, + isGitlabMirror: false, + contributingGuideFile: '', + claLink: '', + }, numberOfCommits: { enabled: true, maxCommitsInfo: 2, maxCommitsWarning: 5 }, prDescription: { enabled: true, minLength: 50, ignoredSections: 'related,release,breaking' }, commitMessages: { @@ -17,11 +23,6 @@ const defaults = { prSize: { enabled: true, maxChangedLines: 1000 }, sourceBranchName: { enabled: true }, targetBranch: { enabled: true }, - // updatedChangelog: { - // enabled: true, - // filename: 'CHANGELOG.md', - // triggers: 'change,feat,fix,remove,revert', - // }, }; /** @@ -30,6 +31,12 @@ const defaults = { * If an environment variable is not set, the system will fall back to the default value from the `defaults` object. */ const config = { + instructions: { + enabled: getEnvBool(process.env.ENABLE_OUTPUT_INSTRUCTIONS) ?? defaults.instructions.enabled, + isGitlabMirror: getEnvBool(process.env.IS_GITLAB_MIRROR) ?? defaults.instructions.isGitlabMirror, + contributingGuideFile: process.env.CONTRIBUTING_GUIDE_FILE || defaults.instructions.contributingGuideFile, + claLink: process.env.CLA_LINK || defaults.instructions.claLink, + }, numberOfCommits: { enabled: getEnvBool(process.env.ENABLE_CHECK_PR_TOO_MANY_COMMITS) ?? defaults.numberOfCommits.enabled, maxCommitsInfo: Number(process.env.MAX_COMMITS) || defaults.numberOfCommits.maxCommitsInfo, @@ -57,11 +64,6 @@ const config = { targetBranch: { enabled: getEnvBool(process.env.ENABLE_CHECK_PR_TARGET_BRANCH) ?? defaults.targetBranch.enabled, }, - // updatedChangelog: { - // enabled: getEnvBool(process.env.ENABLE_CHECK_UPDATED_CHANGELOG) ?? defaults.updatedChangelog.enabled, - // filename: process.env.CHANGELOG_FILENAME || defaults.updatedChangelog.filename, - // triggers: process.env.CHANGELOG_UPDATE_TRIGGERS || defaults.updatedChangelog.triggers, - // }, }; /** @@ -76,15 +78,16 @@ const parametersForTable = [ { ciVar: 'ENABLE_CHECK_PR_SOURCE_BRANCH_NAME', value: config.sourceBranchName.enabled, defaultValue: defaults.sourceBranchName.enabled }, { ciVar: 'ENABLE_CHECK_PR_TARGET_BRANCH', value: config.targetBranch.enabled, defaultValue: defaults.targetBranch.enabled }, { ciVar: 'ENABLE_CHECK_PR_TOO_MANY_COMMITS', value: config.numberOfCommits.enabled, defaultValue: defaults.numberOfCommits.enabled }, - // { ciVar: 'ENABLE_CHECK_UPDATED_CHANGELOG', value: config.updatedChangelog.enabled, defaultValue: defaults.updatedChangelog.enabled }, - // { ciVar: 'CHANGELOG_FILENAME', value: config.updatedChangelog.filename, defaultValue: defaults.updatedChangelog.filename }, - // { ciVar: 'CHANGELOG_UPDATE_TRIGGERS', value: config.updatedChangelog.triggers, defaultValue: defaults.updatedChangelog.triggers }, + { ciVar: 'ENABLE_OUTPUT_INSTRUCTIONS', value: config.instructions.enabled, defaultValue: defaults.instructions.enabled }, + { ciVar: 'CLA_LINK', value: config.instructions.claLink, defaultValue: defaults.instructions.claLink }, { ciVar: 'COMMIT_MESSAGE_ALLOWED_TYPES', value: config.commitMessages.allowedTypes, defaultValue: defaults.commitMessages.allowedTypes }, + { ciVar: 'CONTRIBUTING_GUIDE_FILE', value: config.instructions.contributingGuideFile, defaultValue: defaults.instructions.contributingGuideFile }, { ciVar: 'IGNORED_SECTIONS_DESCRIPTION', value: config.prDescription.ignoredSections, defaultValue: defaults.prDescription.ignoredSections }, + { ciVar: 'IS_GITLAB_MIRROR', value: config.instructions.isGitlabMirror, defaultValue: defaults.instructions.isGitlabMirror }, { ciVar: 'MAX_COMMIT_MESSAGE_BODY_LINE', value: config.commitMessages.maxBodyLineLength, defaultValue: defaults.commitMessages.maxBodyLineLength }, { ciVar: 'MAX_COMMIT_MESSAGE_SUMMARY', value: config.commitMessages.maxSummaryLength, defaultValue: defaults.commitMessages.maxSummaryLength }, - { ciVar: 'MAX_COMMITS', value: config.numberOfCommits.maxCommitsInfo, defaultValue: defaults.numberOfCommits.maxCommitsInfo }, { ciVar: 'MAX_COMMITS_WARN', value: config.numberOfCommits.maxCommitsWarning, defaultValue: defaults.numberOfCommits.maxCommitsWarning }, + { ciVar: 'MAX_COMMITS', value: config.numberOfCommits.maxCommitsInfo, defaultValue: defaults.numberOfCommits.maxCommitsInfo }, { ciVar: 'MAX_PR_LINES', value: config.prSize.maxChangedLines, defaultValue: defaults.prSize.maxChangedLines }, { ciVar: 'MIN_COMMIT_MESSAGE_SUMMARY', value: config.commitMessages.minSummaryLength, defaultValue: defaults.commitMessages.minSummaryLength }, { ciVar: 'MIN_PR_DESCRIPTION_LENGTH', value: config.prDescription.minLength, defaultValue: defaults.prDescription.minLength }, diff --git a/src/dangerfile.ts b/src/dangerfile.ts index a666ac7..b6139fb 100644 --- a/src/dangerfile.ts +++ b/src/dangerfile.ts @@ -6,6 +6,7 @@ import ruleNumberOfCommits from './ruleNumberOfCommits'; import rulePrDescription from './rulePrDescription'; import ruleSourceBranchName from './ruleSourceBranchName'; import ruleTargetBranch from './ruleTargetBranch'; +import outputInstructions from './outputInstructions'; declare const results: DangerResults; declare const message: (message: string, results?: DangerResults) => void; @@ -25,6 +26,9 @@ async function runDangerRules(): Promise { // Show DangerJS individual checks statuses - visible in CI job tracelog displayAllOutputStatuses(); + // Show DangerJS dynamic output message and instructions - visible in feedback comment + await outputInstructions(); + // Show success message if no issues are found const foundIssues: boolean = Boolean(results.fails.length + results.warnings.length + results.messages.length); if (!foundIssues) return message('🎉 Good Job! All checks are passing!'); diff --git a/src/outputInstructions.ts b/src/outputInstructions.ts new file mode 100644 index 0000000..809cad6 --- /dev/null +++ b/src/outputInstructions.ts @@ -0,0 +1,103 @@ +import { DangerDSLType, DangerResults } from 'danger'; +import { Octokit } from '@octokit/rest'; +import { config } from './configParameters'; + +declare const danger: DangerDSLType; +declare const results: DangerResults; +declare const markdown: (message: string, results?: DangerResults) => void; + +/** + * Generates a greeting message and a set of instructions for the author of a Pull Request (PR). + * + * This function creates a custom message for the MR author, providing guidance on how to handle + * the automated feedback from DangerJS. + * It includes a set of recommended actions for resolving warnings and information messages, when issues are found, + * and instructions on how to retry DangerJS checks if necessary. + * Message is dynamically generated based on the type of Danger issues found in the MR. + */ +const prAuthorUsername = danger.github.pr.user.login; +const repositoryOwner = danger.github.pr.base.repo.owner.login; +const repositoryName = danger.github.pr.base.repo.name; +const dangerProjectUrl: string = 'https://github.com/espressif/shared-github-dangerjs'; + +export default async function (): Promise { + // Basic instructions (ALWAYS SHOWN) + let instructions: string = ''; + instructions += `👏 Hi ${prAuthorUsername}, thank you for your Pull Request and contribution to this project!
`; + + // Contributors guide link, if exists in the repository + if (config.instructions.contributingGuideFile) { + const contributionsGuideLink = `https://github.com/${repositoryOwner}/${repositoryName}/blob/${await getDefaultBranch()}/${ + config.instructions.contributingGuideFile + }`; + instructions += `
`; + instructions += `📘 Please check project Contributions Guide of the project for the contribution checklist, information regarding code and documentation style, testing and other topics.
`; + } + + // Contributor License Agreement, if provided link to it + if (config.instructions.claLink) { + instructions += `
`; + instructions += `🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project.
`; + } + + // Basic instructions (ALWAYS SHOWN) + instructions += `
`; + instructions += `
Click to see more instructions ...

`; // START collapsible section INSTRUCTIONS + instructions += `
This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

`; + instructions += `DangerJS is triggered with each "push" event to a Pull Request and modify the contents of this comment.

`; + instructions += `Please consider the following:
`; + instructions += `- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
`; + instructions += `- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
`; + + // If 'warning' or 'error' issues exist, add this to Instructions DangerJS + if (results.fails.length + results.warnings.length) { + instructions += '- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
'; + } + + // If info issues exist, add this to Instructions DangerJS + if (results.messages.length) { + instructions += `- Addressing info messages (📖) is strongly recommended; they're less critical but valuable.
`; + } + + // Add (always) retry link as last line of Instructions DangerJS + const retryLinkUrl: string = `https://github.com/${repositoryOwner}/${repositoryName}/actions`; + instructions += `- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.
`; + instructions += `

`; // END collapsible section INSTRUCTIONS + + // Instructions about pull request Review and Merge process + instructions += `
Review and merge process you can expect ...

`; // START collapsible section REVIEW PROCESS + instructions += `
`; + instructions += `We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.

`; + if (config.instructions.isGitlabMirror) { + instructions += `This GitHub project is public mirror of our internal git repository

`; + instructions += `1. An internal issue has been created for the PR, we assign it to the relevant engineer.
`; + instructions += `2. They review the PR and either approve it or ask you for changes or clarifications.
`; + instructions += `3. Once the GitHub PR is approved, we synchronize it into our internal git repository.
`; + instructions += `4. In the internal git repository we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
`; + instructions += ` - At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
`; + instructions += `5. If the change is approved and passes the tests it is merged into the default branch.
`; + instructions += `5. On next sync from the internal git repository merged change will appear in this public GitHub repository.
`; + } else { + instructions += `1. An internal issue has been created for the PR, we assign it to the relevant engineer.
`; + instructions += `2. They review the PR and either approve it or ask you for changes or clarifications.
`; + instructions += `3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
`; + instructions += ` - At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
`; + instructions += `4. If the change is approved and passes the tests it is merged into the default branch.
`; + } + instructions += `

`; // END collapsible section REVIEW PROCESS + instructions += `\n`; + + // Create output message + return markdown(instructions); +} + +/** + * Fetches the default branch of the repository. + */ +async function getDefaultBranch(): Promise { + const repositoryOwner: string = danger.github.pr.base.repo.owner.login; + const repositoryName: string = danger.github.pr.base.repo.name; + const octokit = new Octokit(); + const { data: repo } = await octokit.repos.get({ owner: repositoryOwner, repo: repositoryName }); + return repo.default_branch; +} diff --git a/src/ruleTargetBranch.ts b/src/ruleTargetBranch.ts index eef5341..3917a0a 100644 --- a/src/ruleTargetBranch.ts +++ b/src/ruleTargetBranch.ts @@ -17,9 +17,9 @@ export default async function (): Promise { if (prTargetBranch !== defaultBranch) { warn(` - The **target branch** for this Pull Request **must be the default branch** of the project (\`${defaultBranch}\`).\n - If you would like to add this feature to a different branch, please state this in the PR description and we will consider it. - `); + The **target branch** for this Pull Request **must be the default branch** of the project (\`${defaultBranch}\`).\n + If you would like to add this feature to a different branch, please state this in the PR description and we will consider it. + `); return recordRuleExitStatus(ruleName, 'Failed'); } diff --git a/tests/outputInstructions.test.ts b/tests/outputInstructions.test.ts new file mode 100644 index 0000000..0bcba71 --- /dev/null +++ b/tests/outputInstructions.test.ts @@ -0,0 +1,249 @@ +import { config as originalConfig } from '../src/configParameters'; + +declare global { + var danger: any; + var results: any; + var markdown: any; +} + +function expectBasicInstructions() { + expect(markdown).toHaveBeenCalledWith(expect.stringContaining('thank you for your Pull Request')); + expect(markdown).toHaveBeenCalledWith(expect.stringContaining('This automated output is generated')); + expect(markdown).toHaveBeenCalledWith(expect.stringContaining('DangerJS is triggered with each "push"')); + expect(markdown).toHaveBeenCalledWith(expect.stringContaining('Please consider the following:')); + expect(markdown).toHaveBeenCalledWith(expect.stringContaining('- Danger mainly focuses on the PR structure and formatting')); + expect(markdown).toHaveBeenCalledWith(expect.stringContaining('not a substitute for human code reviews')); + expect(markdown).toHaveBeenCalledWith(expect.stringContaining('retry these Danger checks')); +} + +function expectContributionsGuideLink(include: boolean = true) { + if (include) expect(markdown).toHaveBeenCalledWith(expect.stringContaining('project Contributions Guide')); + if (!include) expect(markdown).not.toHaveBeenCalledWith(expect.stringContaining('project Contributions Guide')); +} + +function expectResolveWarnings(include: boolean = true) { + if (include) expect(markdown).toHaveBeenCalledWith(expect.stringContaining('Resolve all warnings (⚠️ )')); + if (!include) expect(markdown).not.toHaveBeenCalledWith(expect.stringContaining('Resolve all warnings (⚠️ )')); +} +function expectResolveInfos(include: boolean = true) { + if (include) expect(markdown).toHaveBeenCalledWith(expect.stringContaining('Addressing info messages (📖)')); + if (!include) expect(markdown).not.toHaveBeenCalledWith(expect.stringContaining('Addressing info messages (📖)')); +} +function expectClaLink(include: boolean = true) { + if (include) expect(markdown).toHaveBeenCalledWith(expect.stringContaining('read and signed')); + if (!include) expect(markdown).not.toHaveBeenCalledWith(expect.stringContaining('read and signed')); +} +function expectGitlabMirror(include: boolean = true) { + if (include) expect(markdown).toHaveBeenCalledWith(expect.stringContaining('This GitHub project is public mirror')); + if (!include) expect(markdown).not.toHaveBeenCalledWith(expect.stringContaining('This GitHub project is public mirror')); +} + +describe('TESTS COMPONENT: outputInstructions for GitHub PRs', () => { + let outputInstructions: any; + beforeEach(async () => { + global.danger = { + github: { + pr: { + user: { login: 'JaneDoe' }, // author of Pull Request + base: { + repo: { + owner: { login: 'espressif' }, + name: 'example-repo', + default_branch: 'main', + }, + }, + }, + }, + }; + global.results = { + fails: [], + warnings: [], + messages: [], + }; + global.markdown = jest.fn(); + jest.resetModules(); + jest.mock('@octokit/rest', () => { + return { + Octokit: jest.fn().mockImplementation(() => ({ + // Mock the default branch fetch + repos: { get: jest.fn().mockResolvedValue({ data: { default_branch: 'main' } }) }, + })), + }; + }); + }); + + describe('DEFAULT: GitHub project, no CLA, no Contributions Guide', () => { + beforeEach(async () => { + jest.doMock('../src/configParameters', () => ({ + config: { + ...originalConfig, + }, + })); + outputInstructions = (await import('../src/outputInstructions')).default; + }); + + it('EXPECT PASS: No issues, only basic instructions (always there)', async () => { + await outputInstructions(); + expectBasicInstructions(); + expectGitlabMirror(false); + expectClaLink(false); + expectContributionsGuideLink(false); + expectResolveWarnings(false); + expectResolveInfos(false); + }); + + it('EXPECT FAIL: WARN issues in PR', async () => { + global.results.warnings = [{ message: 'Sample warning' }]; + await outputInstructions(); + expectBasicInstructions(); + expectGitlabMirror(false); + expectClaLink(false); + expectContributionsGuideLink(false); + expectResolveWarnings(); + expectResolveInfos(false); + }); + + it('EXPECT FAIL: ERROR issues in PR', async () => { + global.results.fails = [{ message: 'Sample error' }]; + await outputInstructions(); + expectBasicInstructions(); + expectGitlabMirror(false); + expectClaLink(false); + expectContributionsGuideLink(false); + expectResolveWarnings(); + expectResolveInfos(false); + }); + + it('EXPECT FAIL: MESSAGE (info) issues in PR', async () => { + global.results.messages = [{ message: 'Sample info message' }]; + await outputInstructions(); + expectBasicInstructions(); + expectGitlabMirror(false); + expectClaLink(false); + expectContributionsGuideLink(false); + expectResolveInfos(); + expectResolveWarnings(false); + }); + + it('EXPECT FAIL: WARN and MESSAGE (info) issues in PR', async () => { + global.results.warnings = [{ message: 'Sample warning' }]; + global.results.messages = [{ message: 'Sample info message' }]; + await outputInstructions(); + expectBasicInstructions(); + expectGitlabMirror(false); + expectClaLink(false); + expectContributionsGuideLink(false); + expectResolveInfos(); + expectResolveWarnings(); + }); + }); + describe('CUSTOM: CLA link defined', () => { + beforeEach(async () => { + jest.doMock('../src/configParameters', () => ({ + config: { + ...originalConfig, + instructions: { + claLink: 'https://cla-assistant.io/espressif/esp-idf', + }, + }, + })); + outputInstructions = (await import('../src/outputInstructions')).default; + }); + + it('EXPECT PASS: No issues, only basic instructions (always there)', async () => { + await outputInstructions(); + expectBasicInstructions(); + expectClaLink(); + expectGitlabMirror(false); + expectContributionsGuideLink(false); + expectResolveWarnings(false); + expectResolveInfos(false); + }); + + it('EXPECT FAIL: WARN and MESSAGE (info) issues in PR', async () => { + global.results.warnings = [{ message: 'Sample warning' }]; + global.results.messages = [{ message: 'Sample info message' }]; + await outputInstructions(); + expectBasicInstructions(); + expectClaLink(); + expectGitlabMirror(false); + expectContributionsGuideLink(false); + expectResolveInfos(); + expectResolveWarnings(); + }); + }); + describe('CUSTOM: Contributions Guide defined', () => { + beforeEach(async () => { + jest.doMock('../src/configParameters', () => ({ + config: { + ...originalConfig, + instructions: { + contributingGuideFile: 'CONTRIBUTING.rst', + }, + }, + })); + outputInstructions = (await import('../src/outputInstructions')).default; + }); + + it('EXPECT PASS: No issues, only basic instructions (always there)', async () => { + await outputInstructions(); + expectBasicInstructions(); + expectContributionsGuideLink(); + expect(markdown).toHaveBeenCalledWith(expect.stringContaining('main/CONTRIBUTING.rst')); + expectClaLink(false); + expectGitlabMirror(false); + expectResolveWarnings(false); + expectResolveInfos(false); + }); + + it('EXPECT FAIL: WARN and MESSAGE (info) issues in PR', async () => { + global.results.warnings = [{ message: 'Sample warning' }]; + global.results.messages = [{ message: 'Sample info message' }]; + await outputInstructions(); + expectBasicInstructions(); + expectContributionsGuideLink(); + expect(markdown).toHaveBeenCalledWith(expect.stringContaining('main/CONTRIBUTING.rst')); + expectClaLink(false); + expectGitlabMirror(false); + expectResolveInfos(); + expectResolveWarnings(); + }); + }); + describe('CUSTOM: GitHub project is Gitlab mirror', () => { + beforeEach(async () => { + jest.doMock('../src/configParameters', () => ({ + config: { + ...originalConfig, + instructions: { + isGitlabMirror: true, + }, + }, + })); + outputInstructions = (await import('../src/outputInstructions')).default; + }); + + it('EXPECT PASS: No issues, only basic instructions (always there)', async () => { + await outputInstructions(); + expectBasicInstructions(); + expectGitlabMirror(); + expectClaLink(false); + expectContributionsGuideLink(false); + expectResolveWarnings(false); + expectResolveInfos(false); + }); + + it('EXPECT FAIL: WARN and MESSAGE (info) issues in PR', async () => { + global.results.warnings = [{ message: 'Sample warning' }]; + global.results.messages = [{ message: 'Sample info message' }]; + await outputInstructions(); + expectBasicInstructions(); + expectGitlabMirror(); + expectClaLink(false); + expectContributionsGuideLink(false); + expectResolveInfos(); + expectResolveWarnings(); + }); + }); +}); + +export {};