-
Notifications
You must be signed in to change notification settings - Fork 276
Fix unusable strategy schema for custom jobs + add compiler extraction (Option B) #19405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e78367d
3471bff
62596b0
bed224d
54ea865
4557c58
2140137
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2199,6 +2199,79 @@ func TestBuildCustomJobsSkipsPreActivationJob(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| // TestBuildCustomJobsWithStrategy tests custom jobs with matrix strategy configuration | ||
| func TestBuildCustomJobsWithStrategy(t *testing.T) { | ||
| tmpDir := testutil.TempDir(t, "strategy-test") | ||
|
|
||
| frontmatter := `--- | ||
| on: push | ||
| permissions: | ||
| contents: read | ||
| engine: copilot | ||
| strict: false | ||
| jobs: | ||
| matrix_job: | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| os: [ubuntu-latest, windows-latest] | ||
| node: [18, 20] | ||
| fail-fast: false | ||
| max-parallel: 2 | ||
| steps: | ||
| - run: echo "matrix job" | ||
| simple_job: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - run: echo "simple job" | ||
| --- | ||
|
|
||
| # Test Workflow | ||
|
|
||
| Test content` | ||
|
|
||
| testFile := filepath.Join(tmpDir, "test.md") | ||
| if err := os.WriteFile(testFile, []byte(frontmatter), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| compiler := NewCompiler() | ||
| if err := compiler.CompileWorkflow(testFile); err != nil { | ||
| t.Fatalf("CompileWorkflow() error: %v", err) | ||
| } | ||
|
|
||
| // Read compiled output | ||
| lockFile := filepath.Join(tmpDir, "test.lock.yml") | ||
| content, err := os.ReadFile(lockFile) | ||
| if err != nil { | ||
| t.Fatalf("Failed to read lock file: %v", err) | ||
| } | ||
|
|
||
| yamlStr := string(content) | ||
|
|
||
| // Verify matrix_job has strategy section | ||
| if !strings.Contains(yamlStr, "matrix_job:") { | ||
| t.Error("Expected matrix_job in compiled output") | ||
| } | ||
| if !strings.Contains(yamlStr, "strategy:") { | ||
| t.Error("Expected strategy section in compiled output") | ||
| } | ||
| if !strings.Contains(yamlStr, "matrix:") { | ||
| t.Error("Expected matrix section in compiled output") | ||
| } | ||
| if !strings.Contains(yamlStr, "fail-fast: false") { | ||
| t.Error("Expected fail-fast: false in compiled output") | ||
| } | ||
| if !strings.Contains(yamlStr, "max-parallel: 2") { | ||
| t.Error("Expected max-parallel: 2 in compiled output") | ||
| } | ||
|
|
||
| // Verify simple_job has no strategy | ||
| if !strings.Contains(yamlStr, "simple_job:") { | ||
| t.Error("Expected simple_job in compiled output") | ||
| } | ||
| } | ||
|
Comment on lines
+2252
to
+2273
|
||
|
|
||
| // TestBuildCustomJobsRunsOnForms tests that runs-on string, array, and object forms | ||
| // are all correctly handled in buildCustomJobs. | ||
| func TestBuildCustomJobsRunsOnForms(t *testing.T) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
strategyextraction duplicates the same marshal/indent logic used forpermissionsjust above. This kind of copy/paste tends to diverge over time (formatting, error handling, indentation rules). Consider factoring this into a small helper (e.g.,marshalJobSection(name string, v any) (string, error)) and use it for bothpermissionsandstrategyto keep behavior consistent.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot refactor the strategy type in the JSON schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 2140137 — extracted the inline
strategyobject definition into a named$defs/job_strategytype and replaced the inline definition with"$ref": "#/$defs/job_strategy", following the same pattern as other reusable types in the schema.