From a217edfbc64cb7435b64f641653dded92acab935 Mon Sep 17 00:00:00 2001 From: Christopher Schleiden Date: Mon, 28 Nov 2022 14:55:50 -0800 Subject: [PATCH 01/10] Re-order runs-on value --- actions-languageserver/src/value-providers/runs-on.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/actions-languageserver/src/value-providers/runs-on.ts b/actions-languageserver/src/value-providers/runs-on.ts index 4fda670..d6cdf29 100644 --- a/actions-languageserver/src/value-providers/runs-on.ts +++ b/actions-languageserver/src/value-providers/runs-on.ts @@ -9,8 +9,8 @@ export async function getRunnerLabels( name: string ): Promise { const defaultLabels = [ - "ubuntu-22.04", "ubuntu-latest", + "ubuntu-22.04", "ubuntu-20.04", "ubuntu-18.04", "windows-latest", @@ -24,7 +24,11 @@ export async function getRunnerLabels( "self-hosted", ]; - const repoLabels = await cache.get(`${owner}/${name}/runner-labels`, undefined, () => fetchRunnerLabels(client, owner, name)); + const repoLabels = await cache.get( + `${owner}/${name}/runner-labels`, + undefined, + () => fetchRunnerLabels(client, owner, name) + ); for (const label of defaultLabels) { repoLabels.add(label); } From a32295a09a478c1517b1d7f8e95d774351610ad8 Mon Sep 17 00:00:00 2001 From: Christopher Schleiden Date: Mon, 28 Nov 2022 15:47:05 -0800 Subject: [PATCH 02/10] Use value providers for validation --- actions-languageserver/src/index.ts | 8 +- actions-languageserver/src/on-completion.ts | 45 ++------ actions-languageserver/src/value-providers.ts | 48 ++++++++ actions-languageservice/src/complete.test.ts | 10 +- actions-languageservice/src/complete.ts | 4 +- actions-languageservice/src/validate.ts | 105 ++++++++++++++++-- .../src/value-providers/config.ts | 11 +- 7 files changed, 164 insertions(+), 67 deletions(-) create mode 100644 actions-languageserver/src/value-providers.ts diff --git a/actions-languageserver/src/index.ts b/actions-languageserver/src/index.ts index 8523f5e..2c84793 100644 --- a/actions-languageserver/src/index.ts +++ b/actions-languageserver/src/index.ts @@ -19,6 +19,7 @@ import { } from "./initializationOptions"; import { onCompletion } from "./on-completion"; import { TTLCache } from "./utils/cache"; +import { valueProviders } from "./value-providers"; // Create a connection for the server, using Node's IPC as a transport. // Also include all preview / proposed LSP features. @@ -90,7 +91,10 @@ documents.onDidChangeContent((change) => { }); async function validateTextDocument(textDocument: TextDocument): Promise { - const result = await validate(textDocument); + const result = await validate( + textDocument, + valueProviders(sessionToken, repoWorkspaceMap) + ); connection.sendDiagnostics({ uri: textDocument.uri, diagnostics: result }); } @@ -111,7 +115,7 @@ connection.onCompletion( documents.get(textDocument.uri)!, sessionToken, repos.find((repo) => textDocument.uri.startsWith(repo.workspaceUri)), - cache, + cache ); } ); diff --git a/actions-languageserver/src/on-completion.ts b/actions-languageserver/src/on-completion.ts index 7d5ff07..28f8fb7 100644 --- a/actions-languageserver/src/on-completion.ts +++ b/actions-languageserver/src/on-completion.ts @@ -1,51 +1,20 @@ import { complete } from "@github/actions-languageservice/complete"; -import { WorkflowContext } from "@github/actions-languageservice/context/workflow-context"; -import { Value, ValueProviderConfig } from "@github/actions-languageservice/value-providers/config"; -import { Octokit } from "@octokit/rest"; import { CompletionItem, Position } from "vscode-languageserver"; import { TextDocument } from "vscode-languageserver-textdocument"; import { RepositoryContext } from "./initializationOptions"; import { TTLCache } from "./utils/cache"; -import { getEnvironments } from "./value-providers/job-environment"; -import { getRunnerLabels } from "./value-providers/runs-on"; +import { valueProviders } from "./value-providers"; export async function onCompletion( position: Position, document: TextDocument, sessionToken: string | undefined, repoContext: RepositoryContext | undefined, - cache: TTLCache, + cache: TTLCache ): Promise { - const config: ValueProviderConfig = { - getCustomValues: async (key: string, context: WorkflowContext) => - getCustomValues(key, context, sessionToken, repoContext, cache), - }; - return await complete(document, position, config); -} - - -async function getCustomValues( - key: string, - _: WorkflowContext, - sessionToken: string | undefined, - repo: RepositoryContext | undefined, - cache: TTLCache, -): Promise { - if (!sessionToken || !repo) { - return; - } - - const octokit = new Octokit({ - auth: sessionToken, - }); - - switch (key) { - case "job-environment": { - return await getEnvironments(octokit, cache, repo.owner, repo.name); - } - - case "runs-on": { - return await getRunnerLabels(octokit, cache, repo.owner, repo.name); - } - } + return await complete( + document, + position, + repoContext && valueProviders(sessionToken, repoContext, cache) + ); } diff --git a/actions-languageserver/src/value-providers.ts b/actions-languageserver/src/value-providers.ts new file mode 100644 index 0000000..dbbb60b --- /dev/null +++ b/actions-languageserver/src/value-providers.ts @@ -0,0 +1,48 @@ +import { ValueProviderConfig } from "@github/actions-languageservice"; +import { WorkflowContext } from "@github/actions-languageservice/context/workflow-context"; +import { Value } from "@github/actions-languageservice/value-providers/config"; +import { Octokit } from "@octokit/rest"; +import { RepositoryContext } from "./initializationOptions"; +import { TTLCache } from "./utils/cache"; +import { getEnvironments } from "./value-providers/job-environment"; +import { getRunnerLabels } from "./value-providers/runs-on"; + +export function valueProviders( + sessionToken: string | undefined, + repoContext: RepositoryContext, + cache: TTLCache +): ValueProviderConfig { + return { + "job-environment": passOctoKit( + sessionToken, + repoContext, + cache, + getEnvironments + ), + "runs-on": passOctoKit(sessionToken, repoContext, cache, getRunnerLabels), + }; +} + +function passOctoKit( + sessionToken: string | undefined, + repo: RepositoryContext, + cache: TTLCache, + f: ( + octokit: Octokit, + cache: TTLCache, + owner: string, + name: string + ) => Promise +) { + return async function (context: WorkflowContext): Promise { + if (!sessionToken) { + return []; + } + + const octokit = new Octokit({ + auth: sessionToken, + }); + + return f(octokit, cache, repo.owner, repo.name); + }; +} diff --git a/actions-languageservice/src/complete.test.ts b/actions-languageservice/src/complete.test.ts index d0e55db..1d9e2e2 100644 --- a/actions-languageservice/src/complete.test.ts +++ b/actions-languageservice/src/complete.test.ts @@ -1,7 +1,7 @@ import {complete} from "./complete"; import {WorkflowContext} from "./context/workflow-context"; import {getPositionFromCursor} from "./test-utils/cursor-position"; -import {Value, ValueProviderConfig} from "./value-providers/config"; +import {ValueProviderConfig} from "./value-providers/config"; describe("completion", () => { it("runs-on", async () => { @@ -179,14 +179,10 @@ jobs: it("custom value providers override defaults", async () => { const input = "on: push\njobs:\n build:\n runs-on: |"; - const getCustomValues = async (key: string, _: WorkflowContext): Promise => { - if (key === "runs-on") { + const config: ValueProviderConfig = { + "runs-on": async (_: WorkflowContext) => { return [{label: "my-custom-label"}]; } - return []; - }; - const config: ValueProviderConfig = { - getCustomValues: getCustomValues }; const result = await complete(...getPositionFromCursor(input), config); diff --git a/actions-languageservice/src/complete.ts b/actions-languageservice/src/complete.ts index b205c07..061561a 100644 --- a/actions-languageservice/src/complete.ts +++ b/actions-languageservice/src/complete.ts @@ -104,7 +104,7 @@ async function getValues( const existingValues = getExistingValues(token, parent); if (token?.definition?.key) { - const customValues = await valueProviderConfig?.getCustomValues(token.definition.key, workflowContext); + const customValues = await valueProviderConfig?.[token.definition.key]?.(workflowContext); if (customValues) { return filterAndSortCompletionOptions(customValues, existingValues); @@ -131,7 +131,7 @@ async function getValues( return filterAndSortCompletionOptions(values, existingValues); } -function getExistingValues(token: TemplateToken | null, parent: TemplateToken) { +export function getExistingValues(token: TemplateToken | null, parent: TemplateToken) { // For incomplete YAML, we may only have a parent token if (token) { if (!isString(token)) { diff --git a/actions-languageservice/src/validate.ts b/actions-languageservice/src/validate.ts index cf8ea00..36a7afc 100644 --- a/actions-languageservice/src/validate.ts +++ b/actions-languageservice/src/validate.ts @@ -3,21 +3,27 @@ import {Expr} from "@github/actions-expressions/ast"; import { convertWorkflowTemplate, isBasicExpression, + isSequence, + isString, parseWorkflow, - ParseWorkflowResult + ParseWorkflowResult, + WorkflowTemplate } from "@github/actions-workflow-parser"; import {splitAllowedContext} from "@github/actions-workflow-parser/templates/allowed-context"; import {TemplateToken} from "@github/actions-workflow-parser/templates/tokens/template-token"; import {TokenRange} from "@github/actions-workflow-parser/templates/tokens/token-range"; import {File} from "@github/actions-workflow-parser/workflows/file"; import {TextDocument} from "vscode-languageserver-textdocument"; -import {Diagnostic, DiagnosticSeverity, Range} from "vscode-languageserver-types"; +import {Diagnostic, DiagnosticSeverity, Range, URI} from "vscode-languageserver-types"; import {ContextProviderConfig} from "./context-providers/config"; import {getContext} from "./context-providers/default"; +import {getWorkflowContext, WorkflowContext} from "./context/workflow-context"; import {AccessError, wrapDictionary} from "./expression-validation/error-dictionary"; import {nullTrace} from "./nulltrace"; -import {ValueProviderConfig} from "./value-providers/config"; +import {findToken} from "./utils/find-token"; +import {Value, ValueProviderConfig} from "./value-providers/config"; +import {defaultValueProviders} from "./value-providers/default"; /** * Validates a workflow file @@ -42,11 +48,16 @@ export async function validate( const result: ParseWorkflowResult = parseWorkflow(file.name, [file], nullTrace); if (result.value) { // Errors will be updated in the context - convertWorkflowTemplate(result.context, result.value); - } + const template = convertWorkflowTemplate(result.context, result.value); - // Validate expressions - validateExpressions(diagnostics, result, contextProviderConfig); + // Validate with value providers + if (valueProviderConfig) { + await validateValueProviders(diagnostics, textDocument.uri, template, result, valueProviderConfig); + } + + // Validate expressions + validateExpressions(diagnostics, result, contextProviderConfig); + } // For now map parser errors directly to diagnostics for (const error of result.context.errors.getErrors()) { @@ -91,7 +102,7 @@ function mapRange(range: TokenRange | undefined): Range { } function validateExpressions( - diagnotics: Diagnostic[], + diagnostics: Diagnostic[], result: ParseWorkflowResult, contextProviderConfig: ContextProviderConfig | undefined ) { @@ -130,7 +141,7 @@ function validateExpressions( // result of the evaluation. } catch (e) { if (e instanceof AccessError) { - diagnotics.push({ + diagnostics.push({ message: `Context access might be invalid: ${e.keyName}`, severity: DiagnosticSeverity.Warning, range: mapRange(expression.range) @@ -143,3 +154,79 @@ function validateExpressions( } } } + +async function validateValueProviders( + diagnostics: Diagnostic[], + documentUri: URI, + template: WorkflowTemplate, + result: ParseWorkflowResult, + valueProviderConfig: ValueProviderConfig +) { + if (!result.value) { + return; + } + + // Allowed values coming from the schema have already been validated. Only check if + // a value provider is defined for a token and if it is, validate the values match. + for (const token of TemplateToken.traverse(result.value)) { + if (token.range && token.definition?.key) { + const defKey = token.definition.key; + + let customValues: Value[] | undefined; + + const customValueProvider = valueProviderConfig[defKey]; + if (customValueProvider) { + customValues = await customValueProvider(getProviderContext(documentUri, template, result.value, token)); + } else { + const defaultValueProvider = defaultValueProviders[defKey]; + if (defaultValueProvider) { + customValues = defaultValueProvider(getProviderContext(documentUri, template, result.value, token)); + } + } + + if (customValues) { + if (isSequence(token)) { + for (let i = 0; i < token.count; ++i) { + const entry = token.get(i); + + if (isString(entry)) { + if (!customValues.map(x => x.label).includes(entry.value)) { + diagnostics.push({ + message: `Value '${entry.value}' is not allowed`, + severity: DiagnosticSeverity.Error, + range: mapRange(entry.range) + }); + } + } + } + } + + if (isString(token)) { + if (!customValues.map(x => x.label).includes(token.value)) { + diagnostics.push({ + message: `Value '${token.value}' is not allowed`, + severity: DiagnosticSeverity.Error, + range: mapRange(token.range) + }); + } + } + } + } + } +} + +function getProviderContext( + documentUri: URI, + template: WorkflowTemplate, + root: TemplateToken, + token: TemplateToken +): WorkflowContext { + const {parent, path} = findToken( + { + line: token.range!.start[0], + character: token.range!.start[1] + }, + root + ); + return getWorkflowContext(documentUri, template, path); +} diff --git a/actions-languageservice/src/value-providers/config.ts b/actions-languageservice/src/value-providers/config.ts index ad87506..20fb54c 100644 --- a/actions-languageservice/src/value-providers/config.ts +++ b/actions-languageservice/src/value-providers/config.ts @@ -5,15 +5,8 @@ export interface Value { description?: string; } -export type ValueProvider = () => Value[]; +export type ValueProvider = (context: WorkflowContext) => Promise; export interface ValueProviderConfig { - getCustomValues: (key: string, context: WorkflowContext) => Promise; - getActionInputs?: (owner: string, name: string, ref: string, path?: string) => Promise; -} - -export interface ActionInput { - name: string; - description?: string; - required?: boolean; + [definitionKey: string]: ValueProvider; } From a495432d40feb83760f5919d5470a8fe90128105 Mon Sep 17 00:00:00 2001 From: Christopher Schleiden Date: Mon, 28 Nov 2022 16:04:53 -0800 Subject: [PATCH 03/10] Merge validation code into a single traversal --- actions-languageservice/src/complete.ts | 2 +- actions-languageservice/src/validate.test.ts | 67 ++++++- actions-languageservice/src/validate.ts | 163 +++++++++--------- .../src/value-providers/default.ts | 6 +- .../src/value-providers/needs.ts | 2 +- 5 files changed, 151 insertions(+), 89 deletions(-) diff --git a/actions-languageservice/src/complete.ts b/actions-languageservice/src/complete.ts index 061561a..da3b550 100644 --- a/actions-languageservice/src/complete.ts +++ b/actions-languageservice/src/complete.ts @@ -117,7 +117,7 @@ async function getValues( (parent.definition?.key && defaultValueProviders[parent.definition.key]); if (valueProvider) { - const values = valueProvider(workflowContext); + const values = await valueProvider(workflowContext); return filterAndSortCompletionOptions(values, existingValues); } diff --git a/actions-languageservice/src/validate.test.ts b/actions-languageservice/src/validate.test.ts index 06616e0..ce230e6 100644 --- a/actions-languageservice/src/validate.test.ts +++ b/actions-languageservice/src/validate.test.ts @@ -1,6 +1,7 @@ -import {Diagnostic} from "vscode-languageserver-types"; +import {Diagnostic, DiagnosticSeverity} from "vscode-languageserver-types"; import {createDocument} from "./test-utils/document"; import {validate} from "./validate"; +import {defaultValueProviders} from "./value-providers/default"; describe("validation", () => { it("valid workflow", async () => { @@ -57,4 +58,68 @@ jobs: } } as Diagnostic); }); + + it("single value not returned by value provider", async () => { + const result = await validate( + createDocument( + "wf.yaml", + `on: push +jobs: + build: + runs-on: does-not-exist + steps: + - run: echo` + ), + defaultValueProviders + ); + + expect(result.length).toBe(1); + expect(result[0]).toEqual({ + message: "Value 'does-not-exist' is not valid", + severity: DiagnosticSeverity.Error, + range: { + end: { + character: 27, + line: 3 + }, + start: { + character: 13, + line: 3 + } + } + } as Diagnostic); + }); + + it("value in sequence not returned by value provider", async () => { + const result = await validate( + createDocument( + "wf.yaml", + `on: push +jobs: + build: + runs-on: + - ubuntu-latest + - does-not-exist + steps: + - run: echo` + ), + defaultValueProviders + ); + + expect(result.length).toBe(1); + expect(result[0]).toEqual({ + message: "Value 'does-not-exist' is not valid", + severity: DiagnosticSeverity.Error, + range: { + end: { + character: 20, + line: 5 + }, + start: { + character: 6, + line: 5 + } + } + } as Diagnostic); + }); }); diff --git a/actions-languageservice/src/validate.ts b/actions-languageservice/src/validate.ts index 36a7afc..89683a1 100644 --- a/actions-languageservice/src/validate.ts +++ b/actions-languageservice/src/validate.ts @@ -10,6 +10,8 @@ import { WorkflowTemplate } from "@github/actions-workflow-parser"; import {splitAllowedContext} from "@github/actions-workflow-parser/templates/allowed-context"; +import {BasicExpressionToken} from "@github/actions-workflow-parser/templates/tokens/basic-expression-token"; +import {StringToken} from "@github/actions-workflow-parser/templates/tokens/string-token"; import {TemplateToken} from "@github/actions-workflow-parser/templates/tokens/template-token"; import {TokenRange} from "@github/actions-workflow-parser/templates/tokens/token-range"; import {File} from "@github/actions-workflow-parser/workflows/file"; @@ -50,13 +52,15 @@ export async function validate( // Errors will be updated in the context const template = convertWorkflowTemplate(result.context, result.value); - // Validate with value providers - if (valueProviderConfig) { - await validateValueProviders(diagnostics, textDocument.uri, template, result, valueProviderConfig); - } - - // Validate expressions - validateExpressions(diagnostics, result, contextProviderConfig); + // Validate expressions and value providers + await additionalValidations( + diagnostics, + textDocument.uri, + template, + result.value, + valueProviderConfig, + contextProviderConfig + ); } // For now map parser errors directly to diagnostics @@ -101,86 +105,34 @@ function mapRange(range: TokenRange | undefined): Range { }; } -function validateExpressions( - diagnostics: Diagnostic[], - result: ParseWorkflowResult, - contextProviderConfig: ContextProviderConfig | undefined -) { - if (!result.value) { - return; - } - - // Iterate over the parsed workflow - for (const token of TemplateToken.traverse(result.value)) { - if (isBasicExpression(token)) { - // Validate the expression - for (const expression of token.originalExpressions || [token]) { - const allowedContexts = token.definition?.readerContext || []; - const {namedContexts, functions} = splitAllowedContext(allowedContexts); - - let expr: Expr | undefined; - - try { - const l = new Lexer(expression.expression); - const lr = l.lex(); - - const p = new Parser(lr.tokens, namedContexts, functions); - expr = p.parse(); - } catch { - // Ignore any error here, we should've caught this earlier in the parsing process - continue; - } - - try { - const context = getContext(namedContexts, contextProviderConfig); - - const e = new Evaluator(expr, wrapDictionary(context)); - e.evaluate(); - - // Any invalid context access would've thrown an error via the `ErrorDictionary`, for now we don't have to check the actual - // result of the evaluation. - } catch (e) { - if (e instanceof AccessError) { - diagnostics.push({ - message: `Context access might be invalid: ${e.keyName}`, - severity: DiagnosticSeverity.Warning, - range: mapRange(expression.range) - }); - } else { - // Ignore error - } - } - } - } - } -} - -async function validateValueProviders( +async function additionalValidations( diagnostics: Diagnostic[], documentUri: URI, template: WorkflowTemplate, - result: ParseWorkflowResult, - valueProviderConfig: ValueProviderConfig + root: TemplateToken, + valueProviderConfig: ValueProviderConfig | undefined, + contextProviderConfig: ContextProviderConfig | undefined ) { - if (!result.value) { - return; - } + for (const token of TemplateToken.traverse(root)) { + // If this is an expression, validate it + if (isBasicExpression(token)) { + validateExpression(diagnostics, token, contextProviderConfig); + } - // Allowed values coming from the schema have already been validated. Only check if - // a value provider is defined for a token and if it is, validate the values match. - for (const token of TemplateToken.traverse(result.value)) { - if (token.range && token.definition?.key) { + // Allowed values coming from the schema have already been validated. Only check if + // a value provider is defined for a token and if it is, validate the values match. + if (valueProviderConfig && token.range && token.definition?.key) { const defKey = token.definition.key; let customValues: Value[] | undefined; const customValueProvider = valueProviderConfig[defKey]; if (customValueProvider) { - customValues = await customValueProvider(getProviderContext(documentUri, template, result.value, token)); + customValues = await customValueProvider(getProviderContext(documentUri, template, root, token)); } else { const defaultValueProvider = defaultValueProviders[defKey]; if (defaultValueProvider) { - customValues = defaultValueProvider(getProviderContext(documentUri, template, result.value, token)); + customValues = await defaultValueProvider(getProviderContext(documentUri, template, root, token)); } } @@ -191,11 +143,7 @@ async function validateValueProviders( if (isString(entry)) { if (!customValues.map(x => x.label).includes(entry.value)) { - diagnostics.push({ - message: `Value '${entry.value}' is not allowed`, - severity: DiagnosticSeverity.Error, - range: mapRange(entry.range) - }); + invalidValue(diagnostics, entry); } } } @@ -203,11 +151,7 @@ async function validateValueProviders( if (isString(token)) { if (!customValues.map(x => x.label).includes(token.value)) { - diagnostics.push({ - message: `Value '${token.value}' is not allowed`, - severity: DiagnosticSeverity.Error, - range: mapRange(token.range) - }); + invalidValue(diagnostics, token); } } } @@ -215,6 +159,14 @@ async function validateValueProviders( } } +function invalidValue(diagnostics: Diagnostic[], token: StringToken) { + diagnostics.push({ + message: `Value '${token.value}' is not valid`, + severity: DiagnosticSeverity.Error, + range: mapRange(token.range) + }); +} + function getProviderContext( documentUri: URI, template: WorkflowTemplate, @@ -230,3 +182,48 @@ function getProviderContext( ); return getWorkflowContext(documentUri, template, path); } + +function validateExpression( + diagnostics: Diagnostic[], + token: BasicExpressionToken, + contextProviderConfig: ContextProviderConfig | undefined +) { + // Validate the expression + for (const expression of token.originalExpressions || [token]) { + const allowedContexts = token.definition?.readerContext || []; + const {namedContexts, functions} = splitAllowedContext(allowedContexts); + + let expr: Expr | undefined; + + try { + const l = new Lexer(expression.expression); + const lr = l.lex(); + + const p = new Parser(lr.tokens, namedContexts, functions); + expr = p.parse(); + } catch { + // Ignore any error here, we should've caught this earlier in the parsing process + continue; + } + + try { + const context = getContext(namedContexts, contextProviderConfig); + + const e = new Evaluator(expr, wrapDictionary(context)); + e.evaluate(); + + // Any invalid context access would've thrown an error via the `ErrorDictionary`, for now we don't have to check the actual + // result of the evaluation. + } catch (e) { + if (e instanceof AccessError) { + diagnostics.push({ + message: `Context access might be invalid: ${e.keyName}`, + severity: DiagnosticSeverity.Warning, + range: mapRange(expression.range) + }); + } else { + // Ignore error + } + } + } +} diff --git a/actions-languageservice/src/value-providers/default.ts b/actions-languageservice/src/value-providers/default.ts index 969833d..1b4c877 100644 --- a/actions-languageservice/src/value-providers/default.ts +++ b/actions-languageservice/src/value-providers/default.ts @@ -1,10 +1,10 @@ import {WorkflowContext} from "../context/workflow-context"; -import {Value} from "./config"; +import {Value, ValueProviderConfig} from "./config"; import {needs} from "./needs"; -export const defaultValueProviders: {[key: string]: (workflowContext: WorkflowContext) => Value[]} = { +export const defaultValueProviders: ValueProviderConfig = { needs, - "runs-on": () => + "runs-on": async (_: WorkflowContext) => stringsToValues([ "ubuntu-latest", "ubuntu-18.04", diff --git a/actions-languageservice/src/value-providers/needs.ts b/actions-languageservice/src/value-providers/needs.ts index 5ca3631..8c886b0 100644 --- a/actions-languageservice/src/value-providers/needs.ts +++ b/actions-languageservice/src/value-providers/needs.ts @@ -1,7 +1,7 @@ import {WorkflowContext} from "../context/workflow-context"; import {Value} from "./config"; -export function needs(context: WorkflowContext): Value[] { +export async function needs(context: WorkflowContext): Promise { if (!context.template) { return []; } From 71619bee95dd35026bbd12891235b6244a68cadb Mon Sep 17 00:00:00 2001 From: Christopher Schleiden Date: Mon, 28 Nov 2022 16:08:47 -0800 Subject: [PATCH 04/10] Use cache when validating with value providers --- actions-languageserver/src/index.ts | 6 +++++- actions-languageserver/src/value-providers.ts | 6 +++++- actions-languageserver/src/value-providers/runs-on.ts | 2 ++ 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/actions-languageserver/src/index.ts b/actions-languageserver/src/index.ts index 2c84793..19839e2 100644 --- a/actions-languageserver/src/index.ts +++ b/actions-languageserver/src/index.ts @@ -93,7 +93,11 @@ documents.onDidChangeContent((change) => { async function validateTextDocument(textDocument: TextDocument): Promise { const result = await validate( textDocument, - valueProviders(sessionToken, repoWorkspaceMap) + valueProviders( + sessionToken, + repos.find((repo) => textDocument.uri.startsWith(repo.workspaceUri)), + cache + ) ); connection.sendDiagnostics({ uri: textDocument.uri, diagnostics: result }); diff --git a/actions-languageserver/src/value-providers.ts b/actions-languageserver/src/value-providers.ts index dbbb60b..fc537be 100644 --- a/actions-languageserver/src/value-providers.ts +++ b/actions-languageserver/src/value-providers.ts @@ -9,9 +9,13 @@ import { getRunnerLabels } from "./value-providers/runs-on"; export function valueProviders( sessionToken: string | undefined, - repoContext: RepositoryContext, + repoContext: RepositoryContext | undefined, cache: TTLCache ): ValueProviderConfig { + if (!repoContext) { + return {}; + } + return { "job-environment": passOctoKit( sessionToken, diff --git a/actions-languageserver/src/value-providers/runs-on.ts b/actions-languageserver/src/value-providers/runs-on.ts index d6cdf29..ed869fb 100644 --- a/actions-languageserver/src/value-providers/runs-on.ts +++ b/actions-languageserver/src/value-providers/runs-on.ts @@ -29,9 +29,11 @@ export async function getRunnerLabels( undefined, () => fetchRunnerLabels(client, owner, name) ); + for (const label of defaultLabels) { repoLabels.add(label); } + return Array.from(repoLabels).map((label) => ({ label })); } From a9cdcdba803348abff7a6125007f9dd7f7770965 Mon Sep 17 00:00:00 2001 From: Christopher Schleiden Date: Mon, 28 Nov 2022 16:15:57 -0800 Subject: [PATCH 05/10] Support allowed/suggested value providers --- actions-languageservice/src/complete.test.ts | 9 ++-- actions-languageservice/src/complete.ts | 4 +- actions-languageservice/src/validate.test.ts | 42 ++++++++++++++++-- actions-languageservice/src/validate.ts | 29 ++++++------- .../src/value-providers/config.ts | 10 ++++- .../src/value-providers/default.ts | 43 ++++++++++--------- .../src/value-providers/definition.ts | 2 +- .../src/value-providers/strings-to-values.ts | 5 +++ 8 files changed, 98 insertions(+), 46 deletions(-) create mode 100644 actions-languageservice/src/value-providers/strings-to-values.ts diff --git a/actions-languageservice/src/complete.test.ts b/actions-languageservice/src/complete.test.ts index 1d9e2e2..9d8124b 100644 --- a/actions-languageservice/src/complete.test.ts +++ b/actions-languageservice/src/complete.test.ts @@ -1,7 +1,7 @@ import {complete} from "./complete"; import {WorkflowContext} from "./context/workflow-context"; import {getPositionFromCursor} from "./test-utils/cursor-position"; -import {ValueProviderConfig} from "./value-providers/config"; +import {ValueProviderConfig, ValueProviderKind} from "./value-providers/config"; describe("completion", () => { it("runs-on", async () => { @@ -180,8 +180,11 @@ jobs: const input = "on: push\njobs:\n build:\n runs-on: |"; const config: ValueProviderConfig = { - "runs-on": async (_: WorkflowContext) => { - return [{label: "my-custom-label"}]; + "runs-on": { + kind: ValueProviderKind.SuggestedValues, + get: async (_: WorkflowContext) => { + return [{label: "my-custom-label"}]; + } } }; const result = await complete(...getPositionFromCursor(input), config); diff --git a/actions-languageservice/src/complete.ts b/actions-languageservice/src/complete.ts index da3b550..7471262 100644 --- a/actions-languageservice/src/complete.ts +++ b/actions-languageservice/src/complete.ts @@ -104,7 +104,7 @@ async function getValues( const existingValues = getExistingValues(token, parent); if (token?.definition?.key) { - const customValues = await valueProviderConfig?.[token.definition.key]?.(workflowContext); + const customValues = await valueProviderConfig?.[token.definition.key]?.get(workflowContext); if (customValues) { return filterAndSortCompletionOptions(customValues, existingValues); @@ -117,7 +117,7 @@ async function getValues( (parent.definition?.key && defaultValueProviders[parent.definition.key]); if (valueProvider) { - const values = await valueProvider(workflowContext); + const values = await valueProvider.get(workflowContext); return filterAndSortCompletionOptions(values, existingValues); } diff --git a/actions-languageservice/src/validate.test.ts b/actions-languageservice/src/validate.test.ts index ce230e6..57d0d38 100644 --- a/actions-languageservice/src/validate.test.ts +++ b/actions-languageservice/src/validate.test.ts @@ -59,7 +59,7 @@ jobs: } as Diagnostic); }); - it("single value not returned by value provider", async () => { + it("single value not returned by suggested value provider", async () => { const result = await validate( createDocument( "wf.yaml", @@ -76,7 +76,7 @@ jobs: expect(result.length).toBe(1); expect(result[0]).toEqual({ message: "Value 'does-not-exist' is not valid", - severity: DiagnosticSeverity.Error, + severity: DiagnosticSeverity.Warning, range: { end: { character: 27, @@ -109,7 +109,7 @@ jobs: expect(result.length).toBe(1); expect(result[0]).toEqual({ message: "Value 'does-not-exist' is not valid", - severity: DiagnosticSeverity.Error, + severity: DiagnosticSeverity.Warning, range: { end: { character: 20, @@ -122,4 +122,40 @@ jobs: } } as Diagnostic); }); + + it("single value not returned by allowed value provider", async () => { + const result = await validate( + createDocument( + "wf.yaml", + `on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - run: echo + build: + runs-on: ubuntu-latest + needs: test2 + steps: + - run: echo` + ), + defaultValueProviders + ); + + expect(result.length).toBe(1); + expect(result[0]).toEqual({ + message: "Value 'test2' is not valid", + severity: DiagnosticSeverity.Error, + range: { + end: { + character: 16, + line: 8 + }, + start: { + character: 11, + line: 8 + } + } + } as Diagnostic); + }); }); diff --git a/actions-languageservice/src/validate.ts b/actions-languageservice/src/validate.ts index 89683a1..5209c09 100644 --- a/actions-languageservice/src/validate.ts +++ b/actions-languageservice/src/validate.ts @@ -24,7 +24,7 @@ import {getWorkflowContext, WorkflowContext} from "./context/workflow-context"; import {AccessError, wrapDictionary} from "./expression-validation/error-dictionary"; import {nullTrace} from "./nulltrace"; import {findToken} from "./utils/find-token"; -import {Value, ValueProviderConfig} from "./value-providers/config"; +import {ValueProviderConfig, ValueProviderKind} from "./value-providers/config"; import {defaultValueProviders} from "./value-providers/default"; /** @@ -124,26 +124,23 @@ async function additionalValidations( if (valueProviderConfig && token.range && token.definition?.key) { const defKey = token.definition.key; - let customValues: Value[] | undefined; - - const customValueProvider = valueProviderConfig[defKey]; - if (customValueProvider) { - customValues = await customValueProvider(getProviderContext(documentUri, template, root, token)); - } else { - const defaultValueProvider = defaultValueProviders[defKey]; - if (defaultValueProvider) { - customValues = await defaultValueProvider(getProviderContext(documentUri, template, root, token)); - } + // Try a custom value provider first + let valueProvider = valueProviderConfig[defKey]; + if (!valueProvider) { + // fall back to default + valueProvider = defaultValueProviders[defKey]; } - if (customValues) { + if (valueProvider) { + const customValues = await valueProvider.get(getProviderContext(documentUri, template, root, token)); + if (isSequence(token)) { for (let i = 0; i < token.count; ++i) { const entry = token.get(i); if (isString(entry)) { if (!customValues.map(x => x.label).includes(entry.value)) { - invalidValue(diagnostics, entry); + invalidValue(diagnostics, entry, valueProvider.kind); } } } @@ -151,7 +148,7 @@ async function additionalValidations( if (isString(token)) { if (!customValues.map(x => x.label).includes(token.value)) { - invalidValue(diagnostics, token); + invalidValue(diagnostics, token, valueProvider.kind); } } } @@ -159,10 +156,10 @@ async function additionalValidations( } } -function invalidValue(diagnostics: Diagnostic[], token: StringToken) { +function invalidValue(diagnostics: Diagnostic[], token: StringToken, kind: ValueProviderKind) { diagnostics.push({ message: `Value '${token.value}' is not valid`, - severity: DiagnosticSeverity.Error, + severity: kind === ValueProviderKind.AllowedValues ? DiagnosticSeverity.Error : DiagnosticSeverity.Warning, range: mapRange(token.range) }); } diff --git a/actions-languageservice/src/value-providers/config.ts b/actions-languageservice/src/value-providers/config.ts index 20fb54c..34b7337 100644 --- a/actions-languageservice/src/value-providers/config.ts +++ b/actions-languageservice/src/value-providers/config.ts @@ -5,7 +5,15 @@ export interface Value { description?: string; } -export type ValueProvider = (context: WorkflowContext) => Promise; +export enum ValueProviderKind { + AllowedValues, + SuggestedValues +} + +export type ValueProvider = { + kind: ValueProviderKind; + get: (context: WorkflowContext) => Promise; +}; export interface ValueProviderConfig { [definitionKey: string]: ValueProvider; diff --git a/actions-languageservice/src/value-providers/default.ts b/actions-languageservice/src/value-providers/default.ts index 1b4c877..be59611 100644 --- a/actions-languageservice/src/value-providers/default.ts +++ b/actions-languageservice/src/value-providers/default.ts @@ -1,25 +1,28 @@ import {WorkflowContext} from "../context/workflow-context"; -import {Value, ValueProviderConfig} from "./config"; +import {ValueProviderConfig, ValueProviderKind} from "./config"; import {needs} from "./needs"; +import {stringsToValues} from "./strings-to-values"; export const defaultValueProviders: ValueProviderConfig = { - needs, - "runs-on": async (_: WorkflowContext) => - stringsToValues([ - "ubuntu-latest", - "ubuntu-18.04", - "ubuntu-16.04", - "windows-latest", - "windows-2019", - "windows-2016", - "macos-latest", - "macos-10.15", - "macos-10.14", - "macos-10.13", - "self-hosted" - ]) + needs: { + kind: ValueProviderKind.AllowedValues, + get: needs + }, + "runs-on": { + kind: ValueProviderKind.SuggestedValues, + get: async (_: WorkflowContext) => + stringsToValues([ + "ubuntu-latest", + "ubuntu-18.04", + "ubuntu-16.04", + "windows-latest", + "windows-2019", + "windows-2016", + "macos-latest", + "macos-10.15", + "macos-10.14", + "macos-10.13", + "self-hosted" + ]) + } }; - -export function stringsToValues(labels: string[]): Value[] { - return labels.map(x => ({label: x})); -} diff --git a/actions-languageservice/src/value-providers/definition.ts b/actions-languageservice/src/value-providers/definition.ts index 71aeefb..ad3e5d2 100644 --- a/actions-languageservice/src/value-providers/definition.ts +++ b/actions-languageservice/src/value-providers/definition.ts @@ -5,7 +5,7 @@ import {OneOfDefinition} from "@github/actions-workflow-parser/templates/schema/ import {StringDefinition} from "@github/actions-workflow-parser/templates/schema/string-definition"; import {getWorkflowSchema} from "@github/actions-workflow-parser/workflows/workflow-schema"; import {Value} from "./config"; -import {stringsToValues} from "./default"; +import {stringsToValues} from "./strings-to-values"; export function definitionValues(def: Definition): Value[] { const schema = getWorkflowSchema(); diff --git a/actions-languageservice/src/value-providers/strings-to-values.ts b/actions-languageservice/src/value-providers/strings-to-values.ts new file mode 100644 index 0000000..ff95f87 --- /dev/null +++ b/actions-languageservice/src/value-providers/strings-to-values.ts @@ -0,0 +1,5 @@ +import {Value} from "./config"; + +export function stringsToValues(labels: string[]): Value[] { + return labels.map(x => ({label: x})); +} From f7e9643f01c4167a4027962fddc777158cddfc25 Mon Sep 17 00:00:00 2001 From: Christopher Schleiden Date: Mon, 28 Nov 2022 16:20:31 -0800 Subject: [PATCH 06/10] Export value provider kind --- actions-languageservice/src/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actions-languageservice/src/index.ts b/actions-languageservice/src/index.ts index db8e8e6..d019592 100644 --- a/actions-languageservice/src/index.ts +++ b/actions-languageservice/src/index.ts @@ -2,4 +2,4 @@ export {complete} from "./complete"; export {ContextProviderConfig} from "./context-providers/config"; export {hover} from "./hover"; export {validate} from "./validate"; -export {ValueProviderConfig} from "./value-providers/config"; +export {ValueProviderConfig, ValueProviderKind} from "./value-providers/config"; From 4d930dfc4e02a21abe18e89454eee62fc147a0d2 Mon Sep 17 00:00:00 2001 From: Christopher Schleiden Date: Mon, 28 Nov 2022 16:20:36 -0800 Subject: [PATCH 07/10] Update custom value providers --- actions-languageserver/src/value-providers.ts | 51 +++++++------------ 1 file changed, 17 insertions(+), 34 deletions(-) diff --git a/actions-languageserver/src/value-providers.ts b/actions-languageserver/src/value-providers.ts index fc537be..9e3fc8e 100644 --- a/actions-languageserver/src/value-providers.ts +++ b/actions-languageserver/src/value-providers.ts @@ -1,6 +1,6 @@ import { ValueProviderConfig } from "@github/actions-languageservice"; import { WorkflowContext } from "@github/actions-languageservice/context/workflow-context"; -import { Value } from "@github/actions-languageservice/value-providers/config"; +import { ValueProviderKind } from "@github/actions-languageservice/value-providers/config"; import { Octokit } from "@octokit/rest"; import { RepositoryContext } from "./initializationOptions"; import { TTLCache } from "./utils/cache"; @@ -9,44 +9,27 @@ import { getRunnerLabels } from "./value-providers/runs-on"; export function valueProviders( sessionToken: string | undefined, - repoContext: RepositoryContext | undefined, + repo: RepositoryContext | undefined, cache: TTLCache ): ValueProviderConfig { - if (!repoContext) { + if (!repo || !sessionToken) { return {}; } + const octokit = new Octokit({ + auth: sessionToken, + }); + return { - "job-environment": passOctoKit( - sessionToken, - repoContext, - cache, - getEnvironments - ), - "runs-on": passOctoKit(sessionToken, repoContext, cache, getRunnerLabels), - }; -} - -function passOctoKit( - sessionToken: string | undefined, - repo: RepositoryContext, - cache: TTLCache, - f: ( - octokit: Octokit, - cache: TTLCache, - owner: string, - name: string - ) => Promise -) { - return async function (context: WorkflowContext): Promise { - if (!sessionToken) { - return []; - } - - const octokit = new Octokit({ - auth: sessionToken, - }); - - return f(octokit, cache, repo.owner, repo.name); + "job-environment": { + kind: ValueProviderKind.AllowedValues, + get: (_: WorkflowContext) => + getEnvironments(octokit, cache, repo.owner, repo.name), + }, + "runs-on": { + kind: ValueProviderKind.SuggestedValues, + get: (_: WorkflowContext) => + getRunnerLabels(octokit, cache, repo.owner, repo.name), + }, }; } From aa0103ba5658334afe659bcf9deb7be7865d0558 Mon Sep 17 00:00:00 2001 From: Christopher Schleiden Date: Mon, 28 Nov 2022 16:23:25 -0800 Subject: [PATCH 08/10] Use different error messages for allowed vs suggested validation errors --- actions-languageservice/src/validate.test.ts | 4 ++-- actions-languageservice/src/validate.ts | 21 +++++++++++++++----- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/actions-languageservice/src/validate.test.ts b/actions-languageservice/src/validate.test.ts index 57d0d38..53ebffc 100644 --- a/actions-languageservice/src/validate.test.ts +++ b/actions-languageservice/src/validate.test.ts @@ -75,7 +75,7 @@ jobs: expect(result.length).toBe(1); expect(result[0]).toEqual({ - message: "Value 'does-not-exist' is not valid", + message: "Value 'does-not-exist' might not be valid", severity: DiagnosticSeverity.Warning, range: { end: { @@ -108,7 +108,7 @@ jobs: expect(result.length).toBe(1); expect(result[0]).toEqual({ - message: "Value 'does-not-exist' is not valid", + message: "Value 'does-not-exist' might not be valid", severity: DiagnosticSeverity.Warning, range: { end: { diff --git a/actions-languageservice/src/validate.ts b/actions-languageservice/src/validate.ts index 5209c09..27cd830 100644 --- a/actions-languageservice/src/validate.ts +++ b/actions-languageservice/src/validate.ts @@ -157,11 +157,22 @@ async function additionalValidations( } function invalidValue(diagnostics: Diagnostic[], token: StringToken, kind: ValueProviderKind) { - diagnostics.push({ - message: `Value '${token.value}' is not valid`, - severity: kind === ValueProviderKind.AllowedValues ? DiagnosticSeverity.Error : DiagnosticSeverity.Warning, - range: mapRange(token.range) - }); + switch (kind) { + case ValueProviderKind.AllowedValues: + diagnostics.push({ + message: `Value '${token.value}' is not valid`, + severity: DiagnosticSeverity.Error, + range: mapRange(token.range) + }); + break; + + case ValueProviderKind.SuggestedValues: + diagnostics.push({ + message: `Value '${token.value}' might not be valid`, + severity: DiagnosticSeverity.Warning, + range: mapRange(token.range) + }); + } } function getProviderContext( From 26ebb2ec63f0364bfc8e73bc90c2eba15c8b280b Mon Sep 17 00:00:00 2001 From: Christopher Schleiden Date: Tue, 29 Nov 2022 07:56:07 -0800 Subject: [PATCH 09/10] Add missing break statement --- actions-languageservice/src/validate.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/actions-languageservice/src/validate.ts b/actions-languageservice/src/validate.ts index 27cd830..83e1f20 100644 --- a/actions-languageservice/src/validate.ts +++ b/actions-languageservice/src/validate.ts @@ -172,6 +172,7 @@ function invalidValue(diagnostics: Diagnostic[], token: StringToken, kind: Value severity: DiagnosticSeverity.Warning, range: mapRange(token.range) }); + break; } } From bd315ebf568acf9efd1d87403b75f13910171f2f Mon Sep 17 00:00:00 2001 From: Christopher Schleiden Date: Tue, 29 Nov 2022 08:04:27 -0800 Subject: [PATCH 10/10] Use map for custom values checks --- actions-languageservice/src/validate.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/actions-languageservice/src/validate.ts b/actions-languageservice/src/validate.ts index 83e1f20..88b8bac 100644 --- a/actions-languageservice/src/validate.ts +++ b/actions-languageservice/src/validate.ts @@ -133,13 +133,14 @@ async function additionalValidations( if (valueProvider) { const customValues = await valueProvider.get(getProviderContext(documentUri, template, root, token)); + const customValuesMap = new Set(customValues.map(x => x.label)); if (isSequence(token)) { for (let i = 0; i < token.count; ++i) { const entry = token.get(i); if (isString(entry)) { - if (!customValues.map(x => x.label).includes(entry.value)) { + if (!customValuesMap.has(entry.value)) { invalidValue(diagnostics, entry, valueProvider.kind); } } @@ -147,7 +148,7 @@ async function additionalValidations( } if (isString(token)) { - if (!customValues.map(x => x.label).includes(token.value)) { + if (!customValuesMap.has(token.value)) { invalidValue(diagnostics, token, valueProvider.kind); } }