Merge pull request #19 from github/cschleiden/validate-with-value-providers

Support allowed/suggested value providers and use them for validation

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,14 @@ documents.onDidChangeContent((change) => {
 });
 
 async function validateTextDocument(textDocument: TextDocument): Promise<void> {
-  const result = await validate(textDocument);
+  const result = await validate(
+    textDocument,
+    valueProviders(
+      sessionToken,
+      repos.find((repo) => textDocument.uri.startsWith(repo.workspaceUri)),
+      cache
+    )
+  );
 
   connection.sendDiagnostics({ uri: textDocument.uri, diagnostics: result });
 }
@@ -111,7 +119,7 @@ connection.onCompletion(
       documents.get(textDocument.uri)!,
       sessionToken,
       repos.find((repo) => textDocument.uri.startsWith(repo.workspaceUri)),
-      cache,
+      cache
     );
   }
 );

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 { valueProviders } from "./value-providers";
-import { getRunnerLabels } from "./value-providers/runs-on";
 
 export async function onCompletion(
   position: Position,
   document: TextDocument,
   sessionToken: string | undefined,
   repoContext: RepositoryContext | undefined,
-  cache: TTLCache,
+  cache: TTLCache
 ): Promise<CompletionItem[]> {
-  const config: ValueProviderConfig = {
+  return await complete(
-    getCustomValues: async (key: string, context: WorkflowContext) =>
+    document,
-      getCustomValues(key, context, sessionToken, repoContext, cache),
+    position,
-  };
+    repoContext && valueProviders(sessionToken, repoContext, cache)
-  return await complete(document, position, config);
+  );
-}
-
-
-async function getCustomValues(
-  key: string,
-  _: WorkflowContext,
-  sessionToken: string | undefined,
-  repo: RepositoryContext | undefined,
-  cache: TTLCache,
-): Promise<Value[] | undefined> {
-  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);
-    }
-  }
 }

actions-languageserver/src/value-providers.ts

@@ -0,0 +1,35 @@
+import { ValueProviderConfig } from "@github/actions-languageservice";
+import { WorkflowContext } from "@github/actions-languageservice/context/workflow-context";
+import { ValueProviderKind } 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,
+  repo: RepositoryContext | undefined,
+  cache: TTLCache
+): ValueProviderConfig {
+  if (!repo || !sessionToken) {
+    return {};
+  }
+
+  const octokit = new Octokit({
+    auth: sessionToken,
+  });
+
+  return {
+    "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),
+    },
+  };
+}

actions-languageserver/src/value-providers/runs-on.ts

@@ -9,8 +9,8 @@ export async function getRunnerLabels(
   name: string
 ): Promise<Value[]> {
   const defaultLabels = [
-    "ubuntu-22.04",
     "ubuntu-latest",
+    "ubuntu-22.04",
     "ubuntu-20.04",
     "ubuntu-18.04",
     "windows-latest",
@@ -24,10 +24,16 @@ 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);
   }
+
   return Array.from(repoLabels).map((label) => ({ label }));
 }
 

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, ValueProviderKind} from "./value-providers/config";
 
 describe("completion", () => {
   it("runs-on", async () => {
@@ -179,14 +179,13 @@ 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<Value[] | undefined> => {
-      if (key === "runs-on") {
-        return [{label: "my-custom-label"}];
-      }
-      return [];
-    };
     const config: ValueProviderConfig = {
-      getCustomValues: getCustomValues
+      "runs-on": {
+        kind: ValueProviderKind.SuggestedValues,
+        get: async (_: WorkflowContext) => {
+          return [{label: "my-custom-label"}];
+        }
+      }
     };
     const result = await complete(...getPositionFromCursor(input), config);
 

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]?.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 = valueProvider(workflowContext);
+    const values = await valueProvider.get(workflowContext);
     return filterAndSortCompletionOptions(values, 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)) {

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";

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,104 @@ jobs:
       }
     } as Diagnostic);
   });
+
+  it("single value not returned by suggested 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' might not be valid",
+      severity: DiagnosticSeverity.Warning,
+      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' might not be valid",
+      severity: DiagnosticSeverity.Warning,
+      range: {
+        end: {
+          character: 20,
+          line: 5
+        },
+        start: {
+          character: 6,
+          line: 5
+        }
+      }
+    } 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);
+  });
 });

actions-languageservice/src/validate.ts

@@ -3,21 +3,29 @@ 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 {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";
 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 {ValueProviderConfig, ValueProviderKind} from "./value-providers/config";
+import {defaultValueProviders} from "./value-providers/default";
 
 /**
  * Validates a workflow file
@@ -42,11 +50,18 @@ 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
+      // Validate expressions and value providers
-    validateExpressions(diagnostics, result, contextProviderConfig);
+      await additionalValidations(
+        diagnostics,
+        textDocument.uri,
+        template,
+        result.value,
+        valueProviderConfig,
+        contextProviderConfig
+      );
+    }
 
     // For now map parser errors directly to diagnostics
     for (const error of result.context.errors.getErrors()) {
@@ -90,56 +105,135 @@ function mapRange(range: TokenRange | undefined): Range {
   };
 }
 
-function validateExpressions(
+async function additionalValidations(
-  diagnotics: Diagnostic[],
+  diagnostics: Diagnostic[],
-  result: ParseWorkflowResult,
+  documentUri: URI,
+  template: WorkflowTemplate,
+  root: TemplateToken,
+  valueProviderConfig: ValueProviderConfig | undefined,
   contextProviderConfig: ContextProviderConfig | undefined
 ) {
-  if (!result.value) {
+  for (const token of TemplateToken.traverse(root)) {
-    return;
+    // If this is an expression, validate it
-  }
-
-  // Iterate over the parsed workflow
-  for (const token of TemplateToken.traverse(result.value)) {
     if (isBasicExpression(token)) {
-      // Validate the expression
+      validateExpression(diagnostics, token, contextProviderConfig);
-      for (const expression of token.originalExpressions || [token]) {
+    }
-        const allowedContexts = token.definition?.readerContext || [];
-        const {namedContexts, functions} = splitAllowedContext(allowedContexts);
 
-        let expr: Expr | undefined;
+    // 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;
 
-        try {
+      // Try a custom value provider first
-          const l = new Lexer(expression.expression);
+      let valueProvider = valueProviderConfig[defKey];
-          const lr = l.lex();
+      if (!valueProvider) {
+        // fall back to default
+        valueProvider = defaultValueProviders[defKey];
+      }
 
-          const p = new Parser(lr.tokens, namedContexts, functions);
+      if (valueProvider) {
-          expr = p.parse();
+        const customValues = await valueProvider.get(getProviderContext(documentUri, template, root, token));
-        } catch {
+        const customValuesMap = new Set(customValues.map(x => x.label));
-          // Ignore any error here, we should've caught this earlier in the parsing process
+
-          continue;
+        if (isSequence(token)) {
+          for (let i = 0; i < token.count; ++i) {
+            const entry = token.get(i);
+
+            if (isString(entry)) {
+              if (!customValuesMap.has(entry.value)) {
+                invalidValue(diagnostics, entry, valueProvider.kind);
+              }
+            }
+          }
         }
 
-        try {
+        if (isString(token)) {
-          const context = getContext(namedContexts, contextProviderConfig);
+          if (!customValuesMap.has(token.value)) {
-
+            invalidValue(diagnostics, token, valueProvider.kind);
-          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) {
-            diagnotics.push({
-              message: `Context access might be invalid: ${e.keyName}`,
-              severity: DiagnosticSeverity.Warning,
-              range: mapRange(expression.range)
-            });
-          } else {
-            // Ignore error
           }
         }
       }
     }
   }
 }
+
+function invalidValue(diagnostics: Diagnostic[], token: StringToken, kind: ValueProviderKind) {
+  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)
+      });
+      break;
+  }
+}
+
+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);
+}
+
+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
+      }
+    }
+  }
+}

actions-languageservice/src/value-providers/config.ts

@@ -5,15 +5,16 @@ export interface Value {
   description?: string;
 }
 
-export type ValueProvider = () => Value[];
+export enum ValueProviderKind {
+  AllowedValues,
+  SuggestedValues
+}
+
+export type ValueProvider = {
+  kind: ValueProviderKind;
+  get: (context: WorkflowContext) => Promise<Value[]>;
+};
 
 export interface ValueProviderConfig {
-  getCustomValues: (key: string, context: WorkflowContext) => Promise<Value[] | undefined>;
+  [definitionKey: string]: ValueProvider;
-  getActionInputs?: (owner: string, name: string, ref: string, path?: string) => Promise<ActionInput[]>;
-}
-
-export interface ActionInput {
-  name: string;
-  description?: string;
-  required?: boolean;
 }

actions-languageservice/src/value-providers/default.ts

@@ -1,25 +1,28 @@
 import {WorkflowContext} from "../context/workflow-context";
-import {Value} from "./config";
+import {ValueProviderConfig, ValueProviderKind} from "./config";
 import {needs} from "./needs";
+import {stringsToValues} from "./strings-to-values";
 
-export const defaultValueProviders: {[key: string]: (workflowContext: WorkflowContext) => Value[]} = {
+export const defaultValueProviders: ValueProviderConfig = {
-  needs,
+  needs: {
-  "runs-on": () =>
+    kind: ValueProviderKind.AllowedValues,
-    stringsToValues([
+    get: needs
-      "ubuntu-latest",
+  },
-      "ubuntu-18.04",
+  "runs-on": {
-      "ubuntu-16.04",
+    kind: ValueProviderKind.SuggestedValues,
-      "windows-latest",
+    get: async (_: WorkflowContext) =>
-      "windows-2019",
+      stringsToValues([
-      "windows-2016",
+        "ubuntu-latest",
-      "macos-latest",
+        "ubuntu-18.04",
-      "macos-10.15",
+        "ubuntu-16.04",
-      "macos-10.14",
+        "windows-latest",
-      "macos-10.13",
+        "windows-2019",
-      "self-hosted"
+        "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}));
-}

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();

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<Value[]> {
   if (!context.template) {
     return [];
   }

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}));
+}