Skip to content

Commit

Permalink
ci: lintcommit.js regressions #5515
Browse files Browse the repository at this point in the history
Problem:
Since 8e34c5e, `node lintcommit.js test` fails various tests and CI
fails for those cases too:

    /home/runner/work/aws-toolkit-vscode/aws-toolkit-vscode/.github/workflows/lintcommit.js:80
        } else if (!scope && typeScope.includes('(')) {

Solution:
Revert the changes from 8e34c5e. It's good to avoid code
duplication, but in this case `parsePRTitle()` is not the right
abstraction:
- it doesn't signal failures in a way that is handled by callers
- its return type is awkward (`undefined | string | object`)
- notify.js doesn't actually need `parsePRTitle`, it only needs to check
`startsWith()`.
  • Loading branch information
justinmk3 authored Aug 28, 2024
1 parent 78423ea commit a063f08
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 28 deletions.
16 changes: 14 additions & 2 deletions .github/workflows/lintcommit.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
//

const fs = require('fs')
const { parsePRTitle } = require('./utils')
// This script intentionally avoids github APIs so that:
// 1. it is locally-debuggable
// 2. the CI job is fast ("npm install" is slow)
Expand Down Expand Up @@ -69,7 +68,20 @@ void scopes
* Returns undefined if `title` is valid, else an error message.
*/
function validateTitle(title) {
const { type, scope, subject } = parsePRTitle(title)
const parts = title.split(':')
const subject = parts.slice(1).join(':').trim()

if (title.startsWith('Merge')) {
return undefined
}

if (parts.length < 2) {
return 'missing colon (:) char'
}

const typeScope = parts[0]

const [type, scope] = typeScope.split(/\(([^)]+)\)$/)

if (/\s+/.test(type)) {
return `type contains whitespace: "${type}"`
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/notification.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
name: Notifications

on:
# `pull_request_target` (as opposed to `pull_request`) gives permissions to comment on PRs.
pull_request_target:
branches: [master, feature/*, staging]
# Default = opened + synchronize + reopened.
Expand Down
5 changes: 2 additions & 3 deletions .github/workflows/notify.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

const { parsePRTitle, hasPath, dedupComment } = require('./utils')
const { hasPath, dedupComment } = require('./utils')

const testFilesMessage =
'This pull request modifies files in src/ but no tests were added/updated. Confirm whether tests should be added or ensure the PR description explains why tests are not required.'
Expand Down Expand Up @@ -59,8 +59,7 @@ module.exports = async ({ github, context }) => {
*/
function requiresChangelogMessage(filenames, title) {
try {
const { type } = parsePRTitle(title)
return !hasPath(filenames, '.changes') && (type === 'fix' || type === 'feat')
return !hasPath(filenames, '.changes') && (title.startsWith('fix') || title.startsWith('feat'))
} catch (e) {
console.log(e)
return undefined
Expand Down
23 changes: 0 additions & 23 deletions .github/workflows/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,6 @@
* SPDX-License-Identifier: Apache-2.0
*/

function parsePRTitle(title) {
const parts = title.split(':')
const subject = parts.slice(1).join(':').trim()

if (title.startsWith('Merge')) {
return undefined
}

if (parts.length < 2) {
return 'missing colon (:) char'
}

const typeScope = parts[0]

const [type, scope] = typeScope.split(/\(([^)]+)\)$/)
return {
type,
scope,
subject,
}
}

/**
* Create a comment on a PR if one does not already exist
*/
Expand All @@ -49,7 +27,6 @@ function hasPath(filenames, path) {
}

module.exports = {
parsePRTitle,
dedupComment,
hasPath,
}

0 comments on commit a063f08

Please sign in to comment.