Refactor if-condition to use schema-driven validation and AST-based status function detection (#218)
- Read allowed context from schema definition instead of hardcoded constants - Parse expressions into AST to accurately detect status functions (avoids false positives from string literals) - Export ensureStatusFunction helper that combines checking and wrapping logic - Remove step-if.yml from skipped tests (now passes with accurate detection) - Add tests for if-condition wrapping in hover/completion position mapping
This commit is contained in:
@@ -69,6 +69,59 @@ jobs:
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
it("job-level if condition without status function (gets wrapped)", () => {
|
||||
expect(
|
||||
testMapToExpressionPos(`on: push
|
||||
jobs:
|
||||
build:
|
||||
if: git|hub.event_name == 'push'
|
||||
runs-on: ubuntu-latest`)
|
||||
).toEqual<ExpressionPos>({
|
||||
expression: "success() && (github.event_name == 'push')",
|
||||
position: {line: 0, column: 17}, // "success() && (".length + 3 = 17
|
||||
documentRange: {
|
||||
start: {line: 3, character: 8},
|
||||
end: {line: 3, character: 35} // End of the original condition in the document
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
it("job-level if condition with status function (not wrapped)", () => {
|
||||
expect(
|
||||
testMapToExpressionPos(`on: push
|
||||
jobs:
|
||||
build:
|
||||
if: alw|ays()
|
||||
runs-on: ubuntu-latest`)
|
||||
).toEqual<ExpressionPos>({
|
||||
expression: "always()",
|
||||
position: {line: 0, column: 3},
|
||||
documentRange: {
|
||||
start: {line: 3, character: 8},
|
||||
end: {line: 3, character: 16}
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
it("step-level if condition without status function (gets wrapped)", () => {
|
||||
expect(
|
||||
testMapToExpressionPos(`on: push
|
||||
jobs:
|
||||
build:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- if: steps.test.outc|ome == 'success'
|
||||
run: echo hello`)
|
||||
).toEqual<ExpressionPos>({
|
||||
expression: "success() && (steps.test.outcome == 'success')",
|
||||
position: {line: 0, column: 29}, // Actual position in the wrapped expression
|
||||
documentRange: {
|
||||
start: {line: 5, character: 12},
|
||||
end: {line: 5, character: 43} // End of the original condition in the document
|
||||
}
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
function testMapToExpressionPos(input: string) {
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import {Pos} from "@actions/expressions/lexer";
|
||||
import {ensureStatusFunction} from "@actions/workflow-parser/model/converter/if-condition";
|
||||
import {TemplateToken} from "@actions/workflow-parser/templates/tokens/template-token";
|
||||
import {isBasicExpression, isString} from "@actions/workflow-parser/templates/tokens/type-guards";
|
||||
import {Position, Range as LSPRange} from "vscode-languageserver-textdocument";
|
||||
@@ -42,14 +43,14 @@ export function mapToExpressionPos(token: TemplateToken, position: Position): Ex
|
||||
) {
|
||||
const condition = token.value.trim();
|
||||
if (condition) {
|
||||
// Check if the condition already contains a status function
|
||||
const hasStatusFunction = /\b(success|failure|cancelled|always)\s*\(/.test(condition);
|
||||
const finalCondition = hasStatusFunction ? condition : `success() && (${condition})`;
|
||||
// Ensure the condition has a status function, wrapping if needed
|
||||
const finalCondition = ensureStatusFunction(condition, token.definitionInfo);
|
||||
|
||||
const exprRange = mapRange(token.range);
|
||||
|
||||
// Calculate offset for wrapping
|
||||
const offset = hasStatusFunction ? 0 : "success() && (".length;
|
||||
// Calculate offset: find where the original condition appears in the final expression
|
||||
// If wrapped, it will be after "success() && (", otherwise it's at position 0
|
||||
const offset = finalCondition.indexOf(condition);
|
||||
|
||||
return {
|
||||
expression: finalCondition,
|
||||
|
||||
@@ -1505,4 +1505,174 @@ jobs:
|
||||
expect(result).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("if condition context restrictions", () => {
|
||||
describe("job-level if", () => {
|
||||
it("allows github context", async () => {
|
||||
const input = `
|
||||
on: push
|
||||
jobs:
|
||||
build:
|
||||
if: github.event_name == 'push'
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- run: echo hello`;
|
||||
|
||||
const result = await validate(createDocument("wf.yaml", input));
|
||||
expect(result).toEqual([]);
|
||||
});
|
||||
|
||||
it("allows needs context", async () => {
|
||||
const input = `
|
||||
on: push
|
||||
jobs:
|
||||
a:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- run: echo hello
|
||||
b:
|
||||
needs: a
|
||||
if: needs.a.result == 'success'
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- run: echo hello`;
|
||||
|
||||
const result = await validate(createDocument("wf.yaml", input));
|
||||
expect(result).toEqual([]);
|
||||
});
|
||||
|
||||
it("allows inputs context", async () => {
|
||||
const input = `
|
||||
on:
|
||||
workflow_dispatch:
|
||||
inputs:
|
||||
environment:
|
||||
type: string
|
||||
jobs:
|
||||
build:
|
||||
if: inputs.environment == 'prod'
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- run: echo hello`;
|
||||
|
||||
const result = await validate(createDocument("wf.yaml", input));
|
||||
expect(result).toEqual([]);
|
||||
});
|
||||
|
||||
// Note: vars and matrix contexts are validated at runtime based on their existence
|
||||
// vars context only exists if organization/repository variables are defined
|
||||
// matrix context only exists if a strategy.matrix is defined
|
||||
});
|
||||
|
||||
describe("step-level if", () => {
|
||||
it("allows steps context", async () => {
|
||||
const input = `
|
||||
on: push
|
||||
jobs:
|
||||
build:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- id: setup
|
||||
run: echo hello
|
||||
- if: steps.setup.outcome == 'success'
|
||||
run: echo world`;
|
||||
|
||||
const result = await validate(createDocument("wf.yaml", input));
|
||||
expect(result).toEqual([]);
|
||||
});
|
||||
|
||||
it("allows job context", async () => {
|
||||
const input = `
|
||||
on: push
|
||||
jobs:
|
||||
build:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- if: job.status == 'success'
|
||||
run: echo hello`;
|
||||
|
||||
const result = await validate(createDocument("wf.yaml", input));
|
||||
expect(result).toEqual([]);
|
||||
});
|
||||
|
||||
it("allows runner context", async () => {
|
||||
const input = `
|
||||
on: push
|
||||
jobs:
|
||||
build:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- if: runner.os == 'Linux'
|
||||
run: echo hello`;
|
||||
|
||||
const result = await validate(createDocument("wf.yaml", input));
|
||||
expect(result).toEqual([]);
|
||||
});
|
||||
|
||||
it("allows env context", async () => {
|
||||
const input = `
|
||||
on: push
|
||||
jobs:
|
||||
build:
|
||||
runs-on: ubuntu-latest
|
||||
env:
|
||||
MY_VAR: value
|
||||
steps:
|
||||
- if: env.MY_VAR == 'value'
|
||||
run: echo hello`;
|
||||
|
||||
const result = await validate(createDocument("wf.yaml", input));
|
||||
expect(result).toEqual([]);
|
||||
});
|
||||
|
||||
it("allows matrix context in matrix job", async () => {
|
||||
const input = `
|
||||
on: push
|
||||
jobs:
|
||||
build:
|
||||
strategy:
|
||||
matrix:
|
||||
os: [ubuntu, windows]
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- if: matrix.os == 'ubuntu'
|
||||
run: echo hello`;
|
||||
|
||||
const result = await validate(createDocument("wf.yaml", input));
|
||||
expect(result).toEqual([]);
|
||||
});
|
||||
|
||||
it("allows hashFiles function", async () => {
|
||||
const input = `
|
||||
on: push
|
||||
jobs:
|
||||
build:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- if: hashFiles('**/*.txt') != ''
|
||||
run: echo hello`;
|
||||
|
||||
const result = await validate(createDocument("wf.yaml", input));
|
||||
expect(result).toEqual([]);
|
||||
});
|
||||
|
||||
it("allows all contexts together", async () => {
|
||||
const input = `
|
||||
on: push
|
||||
jobs:
|
||||
build:
|
||||
runs-on: ubuntu-latest
|
||||
env:
|
||||
JOB_VAR: job-value
|
||||
steps:
|
||||
- id: first
|
||||
run: echo hello
|
||||
- if: github.event_name == 'push' && steps.first.outcome == 'success' && job.status == 'success' && runner.os == 'Linux' && env.JOB_VAR == 'job-value'
|
||||
run: echo world`;
|
||||
|
||||
const result = await validate(createDocument("wf.yaml", input));
|
||||
expect(result).toEqual([]);
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -2,6 +2,7 @@ import {Lexer, Parser} from "@actions/expressions";
|
||||
import {Expr} from "@actions/expressions/ast";
|
||||
import {ParseWorkflowResult, WorkflowTemplate, isBasicExpression, isString} from "@actions/workflow-parser";
|
||||
import {ErrorPolicy} from "@actions/workflow-parser/model/convert";
|
||||
import {ensureStatusFunction} from "@actions/workflow-parser/model/converter/if-condition";
|
||||
import {splitAllowedContext} from "@actions/workflow-parser/templates/allowed-context";
|
||||
import {BasicExpressionToken} from "@actions/workflow-parser/templates/tokens/basic-expression-token";
|
||||
import {StringToken} from "@actions/workflow-parser/templates/tokens/string-token";
|
||||
@@ -118,9 +119,8 @@ async function additionalValidations(
|
||||
// Convert the string to an expression token for validation
|
||||
const condition = token.value.trim();
|
||||
if (condition) {
|
||||
// Check if the condition already contains a status function
|
||||
const hasStatusFunction = /\b(success|failure|cancelled|always)\s*\(/.test(condition);
|
||||
const finalCondition = hasStatusFunction ? condition : `success() && (${condition})`;
|
||||
// Ensure the condition has a status function, wrapping if needed
|
||||
const finalCondition = ensureStatusFunction(condition, token.definitionInfo);
|
||||
|
||||
// Create a BasicExpressionToken for validation
|
||||
const expressionToken = new BasicExpressionToken(
|
||||
|
||||
Reference in New Issue
Block a user