batches: add button to automatically exclude repo in batch spec (#24942)

* Add yaml-ast-parser

* Introduce yaml-utils for automagically updating batch spec YAML code to exclude a repo

* Clean up

* Add tests

* Update comments, regex escape repo string

client/web/src/enterprise/batches/create/examples/ExampleTabs.tsx

@@ -1,5 +1,6 @@
 import { Tab, TabList, TabPanel, TabPanels, Tabs, useTabsContext } from '@reach/tabs'
 import classNames from 'classnames'
+import CloseIcon from 'mdi-react/CloseIcon'
 import React, { useState, useCallback, useEffect, useMemo } from 'react'
 import { Subject } from 'rxjs'
 import { catchError, debounceTime, startWith, switchMap } from 'rxjs/operators'
@@ -17,6 +18,7 @@ import { SidebarGroup, SidebarGroupHeader } from '../../../../components/Sidebar
 import { BatchSpecWorkspacesFields } from '../../../../graphql-operations'
 import { MonacoSettingsEditor } from '../../../../settings/MonacoSettingsEditor'
 import { BatchSpecDownloadLink, getFileName } from '../../BatchSpec'
+import { excludeRepo } from '../yaml-util'
 
 import { resolveWorkspacesForBatchSpec } from './backend'
 import combySample from './comby.batch.yaml'
@@ -106,7 +108,30 @@ const ExampleTabPanel: React.FunctionComponent<ExampleTabPanelProps> = ({
     updateSpec,
     ...props
 }) => {
+    const { selectedIndex } = useTabsContext()
+    const isSelected = useMemo(() => selectedIndex === index, [selectedIndex, index])
+
     const [code, setCode] = useState<string>(example.code)
+    const [codeUpdateError, setCodeUpdateError] = useState<string>()
+
+    // Updates the batch spec code when the user wants to exclude a repo resolved in the
+    // workspaces preview.
+    const excludeRepoFromSpec = useCallback(
+        (repo: string, branch: string) => {
+            setCodeUpdateError(undefined)
+
+            const result = excludeRepo(code, repo, branch)
+
+            if (result.success) {
+                setCode(result.spec)
+            } else {
+                setCodeUpdateError(
+                    'Unable to update batch spec. Double-check to make sure there are no syntax errors, then try again.'
+                )
+            }
+        },
+        [code]
+    )
 
     const codeUpdates = useMemo(() => new Subject<string>(), [])
 
@@ -129,14 +154,12 @@ const ExampleTabPanel: React.FunctionComponent<ExampleTabPanelProps> = ({
         )
     )
 
-    const { selectedIndex } = useTabsContext()
-
     // Update the spec in parent state whenever the code changes
     useEffect(() => {
-        if (selectedIndex === index) {
+        if (isSelected) {
             updateSpec({ code, fileName: getFileName(example.name) })
         }
-    }, [code, example.name, updateSpec, selectedIndex, index])
+    }, [code, example.name, isSelected, updateSpec])
 
     const reset = useCallback(() => setCode(example.code), [example.code])
 
@@ -159,16 +182,19 @@ const ExampleTabPanel: React.FunctionComponent<ExampleTabPanelProps> = ({
                 />
             </Container>
             <Container>
-                <h3>Preview workspaces</h3>
-                <PreviewWorkspaces preview={preview} />
+                {codeUpdateError && <ErrorAlert error={codeUpdateError} />}
+                <PreviewWorkspaces excludeRepo={excludeRepoFromSpec} preview={preview} />
             </Container>
         </TabPanel>
     )
 }
 
-const PreviewWorkspaces: React.FunctionComponent<{ preview: BatchSpecWorkspacesFields | Error | undefined }> = ({
-    preview,
-}) => {
+interface PreviewWorkspacesProps {
+    excludeRepo: (repo: string, branch: string) => void
+    preview: BatchSpecWorkspacesFields | Error | undefined
+}
+
+const PreviewWorkspaces: React.FunctionComponent<PreviewWorkspacesProps> = ({ excludeRepo, preview }) => {
     if (isErrorLike(preview)) {
         return <ErrorAlert error={preview} />
     }
@@ -177,31 +203,44 @@ const PreviewWorkspaces: React.FunctionComponent<{ preview: BatchSpecWorkspacesF
     }
     return (
         <>
+            <h3>Preview workspaces ({preview.workspaces.length})</h3>
             <p className="text-monospace">
                 allowUnsupported: {JSON.stringify(preview.allowUnsupported)}
                 <br />
                 allowIgnored: {JSON.stringify(preview.allowIgnored)}
             </p>
-            <ul className="list-group p-1 mb-0">
+            <ul className="list-group mb-0">
                 {preview.workspaces.map(item => (
                     <li
-                        className="list-group-item"
+                        className="d-flex border-bottom mb-3"
                         key={`${item.repository.id}_${item.branch.target.oid}_${item.path || '/'}`}
                     >
-                        <p>
-                            {item.repository.name}:{item.branch.abbrevName}@{item.branch.target.oid} Path:{' '}
-                            {item.path || '/'}
-                        </p>
-                        <p>{item.searchResultPaths.join(', ')}</p>
-                        <ul>
-                            {item.steps.map((step, index) => (
-                                <li key={index}>
-                                    <span className="text-monospace">{step.command}</span>
-                                    <br />
-                                    <span className="text-muted">{step.container}</span>
-                                </li>
-                            ))}
-                        </ul>
+                        <button
+                            className="btn align-self-start p-0 m-0 mr-3"
+                            data-tooltip="Omit this repository from batch spec file"
+                            type="button"
+                            // TODO: Alert that for monorepos, we will exclude all paths
+                            onClick={() => excludeRepo(item.repository.name, item.branch.displayName)}
+                        >
+                            <CloseIcon className="icon-inline" />
+                        </button>
+                        <div className="mb-2 flex-1">
+                            <p>
+                                {item.repository.name}:{item.branch.abbrevName}@{item.branch.target.oid} Path:{' '}
+                                {item.path || '/'}
+                            </p>
+                            <p>{item.searchResultPaths.join(', ')}</p>
+                            <ul>
+                                {item.steps.map((step, index) => (
+                                    // eslint-disable-next-line react/no-array-index-key
+                                    <li key={index}>
+                                        <span className="text-monospace">{step.command}</span>
+                                        <br />
+                                        <span className="text-muted">{step.container}</span>
+                                    </li>
+                                ))}
+                            </ul>
+                        </div>
                     </li>
                 ))}
             </ul>

client/web/src/enterprise/batches/create/yaml-util.test.ts

@@ -0,0 +1,204 @@
+import { excludeRepo } from './yaml-util'
+
+const SAMPLE_SPECS: { original: string; expected: string | 0; repo: string; branch: string }[] = [
+    // Spec with only one "repository" directive, repo to remove doesn't match => no change
+    {
+        repo: 'no-match',
+        branch: 'doesnt-matter',
+        original: `name: hello-world
+on:
+    - repository: repo1
+`,
+        expected: 0,
+    },
+
+    // Spec with multiple "repository" directives, repo to remove doesn't match => no change
+    {
+        repo: 'no-match',
+        branch: 'doesnt-matter',
+        original: `name: hello-world
+on:
+    - repository: repo1
+    - repository: repo2
+    - repository: repo3
+    - repository: repo4
+      branch: doesnt-matter
+`,
+        expected: 0,
+    },
+
+    // Spec with multiple "repository" directives, repo to remove matches => remove it
+    {
+        repo: 'repo1',
+        branch: 'doesnt-matter',
+        original: `name: hello-world
+on:
+    - repository: repo1
+    - repository: repo2
+    - repository: repo3
+      branch: doesnt-matter
+`,
+        expected: `name: hello-world
+on:
+    - repository: repo2
+    - repository: repo3
+      branch: doesnt-matter
+`,
+    },
+
+    // Spec with multiple "repository" directives, repo to remove matches case insensitive => remove it
+    {
+        repo: 'repo1',
+        branch: 'doesnt-matter',
+        original: `name: hello-world
+on:
+    - repository: REPO1
+    - repository: repo2
+`,
+        expected: `name: hello-world
+on:
+    - repository: repo2
+`,
+    },
+
+    // Spec with multiple "repository" directives + branches, repo + branch to remove
+    // matches => remove it
+    {
+        repo: 'repo1',
+        branch: 'branch2',
+        original: `name: hello-world
+on:
+    - repository: repo1
+      branch: branch1
+    - repository: repo1
+      branch: branch2
+    - repository: repo1
+      branch: branch3
+    - repository: repo2
+    - repository: repo3
+      branch: doesnt-matter
+`,
+        expected: `name: hello-world
+on:
+    - repository: repo1
+      branch: branch1
+    - repository: repo1
+      branch: branch3
+    - repository: repo2
+    - repository: repo3
+      branch: doesnt-matter
+`,
+    },
+
+    // Spec with multiple "repository" directives + branches, repo + branch to remove
+    // matches case insensitive => remove it
+    {
+        repo: 'repo1',
+        branch: 'branch2',
+        original: `name: hello-world
+on:
+    - repository: REPO1
+      branch: BRANCH1
+    - repository: REPO1
+      branch: BRANCH2
+`,
+        expected: `name: hello-world
+on:
+    - repository: REPO1
+      branch: BRANCH1
+`,
+    },
+
+    // Spec with multiple "repository" directives + branches, repo + branch to remove
+    // doesn't match => no change
+    {
+        repo: 'repo1',
+        branch: 'no-match',
+        original: `name: hello-world
+on:
+    - repository: repo1
+      branch: branch1
+    - repository: repo1
+      branch: branch2
+    - repository: repo1
+      branch: branch3
+    - repository: repo2
+      branch: no-match
+`,
+        expected: 0,
+    },
+
+    // Spec with "repositoriesMatchingQuery" => append "-repo:"
+    {
+        repo: 'repo1',
+        branch: 'doesnt-matter',
+        original: `name: hello-world
+on:
+    - repositoriesMatchingQuery: file:README.md
+`,
+        expected: `name: hello-world
+on:
+    - repositoriesMatchingQuery: file:README.md -repo:repo1
+`,
+    },
+
+    // Spec with "repositoriesMatchingQuery" and multiple "repository" directives but repo
+    // to remove doesn't match => just append "-repo:"
+    {
+        repo: 'repo1',
+        branch: 'doesnt-matter',
+        original: `name: hello-world
+on:
+    - repositoriesMatchingQuery: file:README.md
+    - repository: repo2
+    - repository: repo3
+`,
+        expected: `name: hello-world
+on:
+    - repositoriesMatchingQuery: file:README.md -repo:repo1
+    - repository: repo2
+    - repository: repo3
+`,
+    },
+
+    // Spec with "repositoriesMatchingQuery" and multiple "repository" directives and repo
+    // to remove matches => append "-repo:" and remove directive
+    {
+        repo: 'repo1',
+        branch: 'doesnt-matter',
+        original: `name: hello-world
+on:
+    - repositoriesMatchingQuery: file:README.md
+    - repository: repo0
+    - repository: repo1
+    - repository: repo2
+`,
+        expected: `name: hello-world
+on:
+    - repositoriesMatchingQuery: file:README.md -repo:repo1
+    - repository: repo0
+    - repository: repo2
+`,
+    },
+]
+
+describe('Batch spec yaml utils', () => {
+    describe('excludeRepo', () => {
+        it('should succeed and exclude the repo from the spec if it can', () => {
+            for (const { original, expected, repo, branch } of SAMPLE_SPECS) {
+                expect(excludeRepo(original, repo, branch)).toEqual({
+                    success: true,
+                    spec: expected === 0 ? original : expected,
+                })
+            }
+        })
+
+        it('should fail and return an error if it cannot parse the spec', () => {
+            expect(excludeRepo('invalid', 'repo1', 'doesnt-matter')).toEqual({
+                success: false,
+                error: 'Spec not parseable',
+                spec: 'invalid',
+            })
+        })
+    })
+})

client/web/src/enterprise/batches/create/yaml-util.ts

@@ -0,0 +1,234 @@
+import { escapeRegExp, find, filter } from 'lodash'
+import { load, Kind as YAMLKind, YamlMap as YAMLMap, YAMLNode, YAMLSequence, YAMLScalar } from 'yaml-ast-parser'
+
+const isYAMLMap = (node: YAMLNode): node is YAMLMap => node.kind === YAMLKind.MAP
+const isYAMLSequence = (node: YAMLNode): node is YAMLSequence => node.kind === YAMLKind.SEQ
+const isYAMLScalar = (node: YAMLNode): node is YAMLScalar => node.kind === YAMLKind.SCALAR
+
+/**
+ * A successful result from manipulating the raw batch spec YAML from its AST, even if the
+ * manipulation was a no-op
+ */
+interface YAMLManipulationSuccess {
+    success: true
+    spec: string
+}
+
+/**
+ * An unsuccessful result from manipulating the raw batch spec YAML from its AST
+ */
+interface YAMLManipulationFailure {
+    success: false
+    error: string
+    spec: string
+}
+
+type YAMLManipulationResult = YAMLManipulationSuccess | YAMLManipulationFailure
+
+/**
+ * Searches the given batch spec YAML AST for a valid "repositoriesMatchingQuery"
+ * directive and, if found, adds "-repo:<repo>" to the query string in order to exclude
+ * the provided repo from the batch spec workspace results
+ *
+ * @param spec the raw batch spec YAML string
+ * @param ast the batch spec YAML loaded as an AST root node, which should be a `YAMLMap`
+ * @param repo the name of the repository to exclude from the batch spec
+ * "repositoriesMatchingQuery"
+ */
+const appendExcludeRepoToQuery = (spec: string, ast: YAMLMap, repo: string): YAMLManipulationResult => {
+    // Find the `YAMLMapping` node with the key "on"
+    const onMapping = find(ast.mappings, mapping => mapping.key.value === 'on')
+    // Take the sequence of values for the "on" key
+    const onSequence = onMapping?.value
+
+    if (!onSequence || !isYAMLSequence(onSequence)) {
+        return { spec, success: false, error: 'Non-sequence value found for "on" key' }
+    }
+
+    // From the sequence, look for a `YAMLMap` node that contains a `YAMLMapping` node
+    // with the key "repositoriesMatchingQuery"
+    const queryMap: YAMLNode | undefined = find(
+        onSequence.items,
+        item => isYAMLMap(item) && !!find(item.mappings, mapping => mapping.key.value === 'repositoriesMatchingQuery')
+    )
+
+    if (!queryMap || !isYAMLMap(queryMap)) {
+        // This just means there wasn't a "repositoriesMatchingQuery" node in the "on"
+        // sequence, so return the unaltered spec
+        return { spec, success: true }
+    }
+
+    // Extract the `YAMLMapping` node with the key "repositoriesMatchingQuery"
+    const queryMapping = find(queryMap.mappings, mapping => mapping.key.value === 'repositoriesMatchingQuery')
+    // Take the value for the "repositoriesMatchingQuery" key -- this should be a
+    // `YAMLScalar` for the query search string
+    const queryValue = queryMapping?.value
+
+    if (!queryValue || !isYAMLScalar(queryValue)) {
+        return { spec, success: false, error: 'Non-scalar value found for "repositoriesMatchingQuery" key' }
+    }
+
+    // Insert "-repo:" qualifier at the end of the query string
+    // TODO: In the future this can be integrated into the batch spec under its own
+    // "excludes" keyword instead
+    return {
+        success: true,
+        spec:
+            spec.slice(0, queryValue.endPosition) + ` -repo:${escapeRegExp(repo)}` + spec.slice(queryValue.endPosition),
+    }
+}
+
+/**
+ * Searches the given batch spec YAML AST for any valid "repository" directives that match
+ * the provided repo name (and branch name, if applicable) and, if found, removes the
+ * directive
+ *
+ * @param spec the raw batch spec YAML string
+ * @param ast the batch spec YAML loaded as an AST root node, which should be a `YAMLMap`
+ * @param repo the name of the repository to omit the "repository" directive for
+ * @param branch the name of the repository branch to omit the "repository" directive for
+ */
+const removeRepoDirective = (spec: string, ast: YAMLMap, repo: string, branch: string): YAMLManipulationResult => {
+    // Find the `YAMLMapping` node with the key "on"
+    const onMapping = find(ast.mappings, mapping => mapping.key.value === 'on')
+    // Take the sequence of values for the "on" key
+    const onSequence = onMapping?.value
+
+    if (!onSequence || !isYAMLSequence(onSequence)) {
+        return { spec, success: false, error: 'Non-sequence value found for "on" key' }
+    }
+
+    // From the sequence, filter to any `YAMLMap` nodes that contain a `YAMLMapping` node
+    // with the key "repository" and a `YAMLScalar` value whose value matches the repo
+    // name (there may be none, one, or multiple `YAMLMap`s for different branches)
+    const repositoryMatchMaps: YAMLMap[] = filter(
+        onSequence.items,
+        (item): item is YAMLMap =>
+            isYAMLMap(item) &&
+            !!find(
+                item.mappings,
+                mapping =>
+                    mapping.key.value === 'repository' &&
+                    isYAMLScalar(mapping.value) &&
+                    // Compare the values case-insensitively
+                    mapping.value.value.toLowerCase() === repo.toLowerCase()
+            )
+    )
+
+    if (repositoryMatchMaps.length === 0) {
+        // This just means there wasn't a matching "repository" directive in the "on"
+        // sequence, so return the unaltered spec
+        return { spec, success: true }
+    }
+
+    // If there's only one matching `YAMLMap` node, we can just remove it from the spec
+    if (repositoryMatchMaps.length === 1) {
+        const repositoryMatchMap = repositoryMatchMaps[0]
+        return {
+            success: true,
+            spec:
+                // NOTE: We also need to trim the sequence delimiter, which is not
+                // included in the `YAMLMap`'s start position to end position range
+                trimLastSequenceItemDelimiter(spec.slice(0, repositoryMatchMap.startPosition)) +
+                spec.slice(repositoryMatchMap.endPosition),
+        }
+    }
+
+    // Otherwise, if there are multiple matches, look for one that contains a
+    // `YAMLMapping` node with the key "branch" and a `YAMLScalar` value whose value
+    // matches the branch argument name
+    const branchMatchMap: YAMLMap | undefined = find(
+        repositoryMatchMaps,
+        map =>
+            !!find(
+                map.mappings,
+                mapping =>
+                    mapping.key.value === 'branch' &&
+                    isYAMLScalar(mapping.value) &&
+                    // Compare the values case-insensitively
+                    mapping.value.value.toLowerCase() === branch.toLowerCase()
+            )
+    )
+
+    // If we found no branch match
+    if (!branchMatchMap) {
+        // This just means none of the matching "repository" directives also matched in
+        // the "branch" specified, so return the unaltered spec
+        return { spec, success: true }
+    }
+
+    // Otherwise, remove the matching `YAMLMap` node from the spec
+    return {
+        success: true,
+        spec:
+            // NOTE: We also need to trim the sequence delimiter, which is not included in
+            // the `YAMLMap`'s start position to end position range
+            trimLastSequenceItemDelimiter(spec.slice(0, branchMatchMap.startPosition)) +
+            spec.slice(branchMatchMap.endPosition),
+    }
+}
+
+/**
+ * Trims the final sequence delimiter (i.e. a set of newlines, spaces, and a dash) from
+ * the given slice of raw batch spec.
+ *
+ * This is "sorry-pls-don't-hate-me"-level hack but unfortunately the easiest way around a
+ * limitation of working with the YAML AST. The YAML AST parser will not include these
+ * characters itself in the character "range" for a node, i.e. they will be present an
+ * indeterminate number of characters before `node.startPosition`. So removing a node from
+ * a sequence in the spec without also invoking this helper would leave an "empty"
+ * sequence item behind and result in parsing errors, like:
+ *
+ * ```yaml
+ * on:
+ *   - repository: github.com/sourcegraph/sourcegraph
+ *   -
+ *   - repository: github.com/sourcegraph/about
+ * ```
+ *
+ * @param specSlice the slice of a raw batch spec YAML string from the beginning up to and
+ * including the last set of sequence delimiter characters (one or more newlines followed
+ * by zero or more spaces, then a single dash, and then zero or more spaces)
+ */
+const trimLastSequenceItemDelimiter = (specSlice: string): string =>
+    // Trim the last instance of one or more newlines, zero or more spaces, a single dash,
+    // and then zero or more spaces, e.g. "\n  - "
+    specSlice.replace(/\n+\s*-\s*$/, '')
+
+/**
+ * Modifies the provided raw batch spec YAML string in order to exclude a repo resolved in
+ * the workspaces preview from the "repositoriesMatchingQuery" value and remove any single
+ * "repository" directive that matches the repo name (and branch name, if applicable).
+ *
+ * @param spec the raw batch spec YAML string
+ * @param repo the name of the repository to omit from the batch spec
+ * @param branch the name of the repository branch to match when omitting from the batch
+ * spec
+ */
+export const excludeRepo = (spec: string, repo: string, branch: string): YAMLManipulationResult => {
+    let ast = load(spec)
+
+    if (!isYAMLMap(ast) || ast.errors.length > 0) {
+        return { spec, success: false, error: 'Spec not parseable' }
+    }
+
+    // First, try to update the "repositoriesMatchingQuery" string with "-repo:<repo>"
+    const appendToQueryResult = appendExcludeRepoToQuery(spec, ast, repo)
+
+    if (!appendToQueryResult.success) {
+        return appendToQueryResult
+    }
+
+    // Re-parse the AST from the updated result
+    ast = load(appendToQueryResult.spec)
+
+    if (!isYAMLMap(ast) || ast.errors.length > 0) {
+        return { spec, success: false, error: 'Could not parse spec after updating "repositoriesMatchingQuery"' }
+    }
+
+    // Then, also update in case we need to remove any single repository directives that
+    // match the repo and branch name
+    const removeRepoResult = removeRepoDirective(appendToQueryResult.spec, ast, repo, branch)
+
+    return removeRepoResult
+}

package.json

@@ -405,6 +405,7 @@
     "webext-domain-permission-toggle": "^1.0.1",
     "webext-patterns": "^0.9.0",
     "webextension-polyfill": "^0.6.0",
+    "yaml-ast-parser": "^0.0.43",
     "zustand": "^3.5.10"
   },
   "resolutions": {