diff --git a/client/web/src/enterprise/batches/create/examples/ExampleTabs.tsx b/client/web/src/enterprise/batches/create/examples/ExampleTabs.tsx index d78c0e777a8..181cd397008 100644 --- a/client/web/src/enterprise/batches/create/examples/ExampleTabs.tsx +++ b/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 = ({ updateSpec, ...props }) => { + const { selectedIndex } = useTabsContext() + const isSelected = useMemo(() => selectedIndex === index, [selectedIndex, index]) + const [code, setCode] = useState(example.code) + const [codeUpdateError, setCodeUpdateError] = useState() + + // 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(), []) @@ -129,14 +154,12 @@ const ExampleTabPanel: React.FunctionComponent = ({ ) ) - 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 = ({ /> -

Preview workspaces

- + {codeUpdateError && } + ) } -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 = ({ excludeRepo, preview }) => { if (isErrorLike(preview)) { return } @@ -177,31 +203,44 @@ const PreviewWorkspaces: React.FunctionComponent<{ preview: BatchSpecWorkspacesF } return ( <> +

Preview workspaces ({preview.workspaces.length})

allowUnsupported: {JSON.stringify(preview.allowUnsupported)}
allowIgnored: {JSON.stringify(preview.allowIgnored)}

-
    +
      {preview.workspaces.map(item => (
    • -

      - {item.repository.name}:{item.branch.abbrevName}@{item.branch.target.oid} Path:{' '} - {item.path || '/'} -

      -

      {item.searchResultPaths.join(', ')}

      -
        - {item.steps.map((step, index) => ( -
      • - {step.command} -
        - {step.container} -
        • - ))} -
      + +
      +

      + {item.repository.name}:{item.branch.abbrevName}@{item.branch.target.oid} Path:{' '} + {item.path || '/'} +

      +

      {item.searchResultPaths.join(', ')}

      +
        + {item.steps.map((step, index) => ( + // eslint-disable-next-line react/no-array-index-key +
      • + {step.command} +
        + {step.container} +
        • + ))} +
      +
      • ))}
    diff --git a/client/web/src/enterprise/batches/create/yaml-util.test.ts b/client/web/src/enterprise/batches/create/yaml-util.test.ts new file mode 100644 index 00000000000..94a3a1cec65 --- /dev/null +++ b/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', + }) + }) + }) +}) diff --git a/client/web/src/enterprise/batches/create/yaml-util.ts b/client/web/src/enterprise/batches/create/yaml-util.ts new file mode 100644 index 00000000000..3f082b9c823 --- /dev/null +++ b/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:" 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:" + 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 +} diff --git a/package.json b/package.json index e2932afbcfb..cd2e1638476 100644 --- a/package.json +++ b/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": {