-
Notifications
You must be signed in to change notification settings - Fork 317
feat: auto-generate --allow-host-service-ports from services: port mappings #23760
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
5199712
51a51ae
4ed5092
6634b44
7aa6405
4938adf
b23bcb5
047ebd0
816a57a
fc65fca
bd6a176
dadfa9e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| --- | ||
| description: Smoke test to validate --allow-host-service-ports with Redis service container | ||
| on: | ||
| workflow_dispatch: | ||
| schedule: daily | ||
| status-comment: true | ||
| permissions: | ||
| contents: read | ||
| issues: read | ||
| pull-requests: read | ||
| name: Smoke Service Ports | ||
| engine: copilot | ||
| strict: true | ||
| services: | ||
| redis: | ||
| image: redis:7 | ||
| ports: | ||
| - 6379:6379 | ||
| options: >- | ||
| --health-cmd "redis-cli ping" | ||
| --health-interval 10s | ||
| --health-timeout 5s | ||
| --health-retries 5 | ||
| network: | ||
| allowed: | ||
| - defaults | ||
| - github | ||
| tools: | ||
| bash: | ||
| - "*" | ||
| safe-outputs: | ||
| allowed-domains: [default-safe-outputs] | ||
| add-comment: | ||
| hide-older-comments: true | ||
| max: 2 | ||
| messages: | ||
| footer: "> 🔌 *Service ports validation by [{workflow_name}]({run_url})*{history_link}" | ||
| run-started: "🔌 Starting service ports validation... [{workflow_name}]({run_url}) is testing Redis connectivity..." | ||
| run-success: "✅ Service ports validation passed! [{workflow_name}]({run_url}) confirms agent can reach Redis." | ||
| run-failure: "❌ Service ports validation failed! [{workflow_name}]({run_url}) could not reach Redis: {status}" | ||
| timeout-minutes: 5 | ||
| --- | ||
|
|
||
| # Smoke Test: Service Ports (Redis) | ||
|
|
||
| **Purpose:** Validate that the `--allow-host-service-ports` feature works end-to-end. The compiler should have automatically detected the Redis service port and configured AWF to allow traffic to it. | ||
|
|
||
| **IMPORTANT:** Inside AWF's sandbox, you must connect to services via `host.docker.internal` (not `localhost`). The service containers run on the host, and AWF routes traffic through the host gateway. Since the workflow maps port 6379:6379, port 6379 should work. Keep all outputs concise. | ||
|
|
||
| ## Required Tests | ||
|
|
||
| 1. **Redis PING**: Run `redis-cli -h host.docker.internal -p 6379 ping` or `echo PING | nc host.docker.internal 6379` and verify the response contains `PONG`. | ||
|
|
||
| 2. **Redis SET/GET**: Write a value to Redis and read it back: | ||
| - `redis-cli -h host.docker.internal -p 6379 SET smoke_test "service-ports-ok"` | ||
| - `redis-cli -h host.docker.internal -p 6379 GET smoke_test` | ||
| - Verify the returned value is `service-ports-ok` | ||
|
|
||
| 3. **Redis INFO**: Run `redis-cli -h host.docker.internal -p 6379 INFO server | head -5` to verify we can query Redis server info. | ||
|
|
||
| ## Output Requirements | ||
|
|
||
| Add a **concise comment** to the pull request (if triggered by PR) with: | ||
|
|
||
| - Each test with a pass/fail status | ||
| - Overall status: PASS or FAIL | ||
| - Note whether `redis-cli` was available or if `nc`/netcat was used as fallback | ||
|
|
||
| Example: | ||
| ``` | ||
| ## Service Ports Smoke Test (Redis) | ||
|
|
||
| | Test | Status | | ||
| |------|--------| | ||
| | Redis PING | ✅ PONG received | | ||
| | Redis SET/GET | ✅ Value round-tripped | | ||
| | Redis INFO | ✅ Server info retrieved | | ||
|
|
||
| **Result:** 3/3 tests passed ✅ | ||
| ``` | ||
|
|
||
| **Important**: If no action is needed after completing your analysis, you **MUST** call the `noop` safe-output tool with a brief explanation. | ||
|
|
||
| ```json | ||
| {"noop": {"message": "No action needed: [brief explanation of what was analyzed and why]"}} | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| //go:build integration | ||
|
|
||
| package cli | ||
|
|
||
| import ( | ||
| "os" | ||
| "os/exec" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" | ||
| ) | ||
|
|
||
| // TestCompileServicePortsWorkflow compiles the canonical test-service-ports.md workflow | ||
| // and verifies that the generated lock file contains --allow-host-service-ports with the | ||
| // correct ${{ job.services['<id>'].ports['<port>'] }} expressions for every service port. | ||
| func TestCompileServicePortsWorkflow(t *testing.T) { | ||
| setup := setupIntegrationTest(t) | ||
| defer setup.cleanup() | ||
|
|
||
| srcPath := filepath.Join(projectRoot, "pkg/cli/workflows/test-service-ports.md") | ||
| dstPath := filepath.Join(setup.workflowsDir, "test-service-ports.md") | ||
|
|
||
| srcContent, err := os.ReadFile(srcPath) | ||
| if err != nil { | ||
| t.Fatalf("Failed to read source workflow %s: %v", srcPath, err) | ||
| } | ||
| if err := os.WriteFile(dstPath, srcContent, 0644); err != nil { | ||
| t.Fatalf("Failed to write workflow to test dir: %v", err) | ||
| } | ||
|
|
||
| cmd := exec.Command(setup.binaryPath, "compile", dstPath) | ||
| output, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| t.Fatalf("Compile failed: %v\nOutput: %s", err, string(output)) | ||
| } | ||
|
|
||
| lockFilePath := filepath.Join(setup.workflowsDir, "test-service-ports.lock.yml") | ||
| lockContent, err := os.ReadFile(lockFilePath) | ||
| if err != nil { | ||
| t.Fatalf("Failed to read lock file: %v", err) | ||
| } | ||
| lock := string(lockContent) | ||
|
|
||
| // The compiler must emit --allow-host-service-ports | ||
| if !strings.Contains(lock, "--allow-host-service-ports") { | ||
| t.Errorf("Lock file missing --allow-host-service-ports\nLock content:\n%s", lock) | ||
| } | ||
|
|
||
| // Bracket-notation expressions must be present for both services | ||
| for _, expr := range []string{ | ||
| "job.services['postgres'].ports['5432']", | ||
| "job.services['redis'].ports['6379']", | ||
| } { | ||
| if !strings.Contains(lock, expr) { | ||
| t.Errorf("Lock file missing expected expression %q\nLock content:\n%s", expr, lock) | ||
| } | ||
| } | ||
|
|
||
| t.Logf("test-service-ports.md compiled successfully; --allow-host-service-ports verified") | ||
| } | ||
|
|
||
| // TestCompileServicePorts_NoServices verifies that a workflow with no services block | ||
| // compiles without errors and does NOT emit --allow-host-service-ports. | ||
| func TestCompileServicePorts_NoServices(t *testing.T) { | ||
| setup := setupIntegrationTest(t) | ||
| defer setup.cleanup() | ||
|
|
||
| testWorkflow := `--- | ||
| on: | ||
| workflow_dispatch: | ||
| permissions: | ||
| contents: read | ||
| engine: copilot | ||
| --- | ||
|
|
||
| # No Services Workflow | ||
|
|
||
| This workflow has no services block and should not include --allow-host-service-ports. | ||
| ` | ||
| testPath := filepath.Join(setup.workflowsDir, "no-services.md") | ||
| if err := os.WriteFile(testPath, []byte(testWorkflow), 0644); err != nil { | ||
| t.Fatalf("Failed to write workflow: %v", err) | ||
| } | ||
|
|
||
| cmd := exec.Command(setup.binaryPath, "compile", testPath) | ||
| output, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| t.Fatalf("Compile failed: %v\nOutput: %s", err, string(output)) | ||
| } | ||
|
|
||
| lockFilePath := filepath.Join(setup.workflowsDir, "no-services.lock.yml") | ||
| lockContent, err := os.ReadFile(lockFilePath) | ||
| if err != nil { | ||
| t.Fatalf("Failed to read lock file: %v", err) | ||
| } | ||
|
|
||
| if strings.Contains(string(lockContent), "--allow-host-service-ports") { | ||
| t.Errorf("Lock file should NOT contain --allow-host-service-ports when no services are defined") | ||
| } | ||
| } | ||
|
|
||
| // TestCompileServicePorts_HyphenatedServiceID verifies that service IDs containing | ||
| // hyphens are emitted with bracket notation (not dot notation) in the compiled lock file. | ||
| func TestCompileServicePorts_HyphenatedServiceID(t *testing.T) { | ||
| setup := setupIntegrationTest(t) | ||
| defer setup.cleanup() | ||
|
|
||
| testWorkflow := `--- | ||
| on: | ||
| workflow_dispatch: | ||
| permissions: | ||
| contents: read | ||
| engine: copilot | ||
| services: | ||
| my-postgres: | ||
| image: postgres:15 | ||
| ports: | ||
| - 5432:5432 | ||
| --- | ||
|
|
||
| # Hyphenated Service ID Workflow | ||
|
|
||
| Verifies bracket notation for hyphenated service IDs. | ||
| ` | ||
| testPath := filepath.Join(setup.workflowsDir, "hyphenated-service.md") | ||
| if err := os.WriteFile(testPath, []byte(testWorkflow), 0644); err != nil { | ||
| t.Fatalf("Failed to write workflow: %v", err) | ||
| } | ||
|
|
||
| cmd := exec.Command(setup.binaryPath, "compile", testPath) | ||
| output, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| t.Fatalf("Compile failed: %v\nOutput: %s", err, string(output)) | ||
| } | ||
|
|
||
| lockFilePath := filepath.Join(setup.workflowsDir, "hyphenated-service.lock.yml") | ||
| lockContent, err := os.ReadFile(lockFilePath) | ||
| if err != nil { | ||
| t.Fatalf("Failed to read lock file: %v", err) | ||
| } | ||
| lock := string(lockContent) | ||
|
|
||
| // Must use bracket notation, not dot notation | ||
| bracketNotation := "job.services['my-postgres'].ports['5432']" | ||
| dotNotation := "job.services.my-postgres.ports" | ||
|
|
||
| if !strings.Contains(lock, bracketNotation) { | ||
| t.Errorf("Lock file missing bracket-notation expression %q\nLock content:\n%s", bracketNotation, lock) | ||
| } | ||
| if strings.Contains(lock, dotNotation) { | ||
| t.Errorf("Lock file must NOT use dot notation for hyphenated service IDs; found %q\nLock content:\n%s", dotNotation, lock) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| --- | ||
| on: | ||
| workflow_dispatch: | ||
| permissions: | ||
| contents: read | ||
| engine: copilot | ||
| services: | ||
| postgres: | ||
| image: postgres:15 | ||
| ports: | ||
| - 5432:5432 | ||
| redis: | ||
| image: redis:7 | ||
| ports: | ||
| - 6379:6379 | ||
| --- | ||
|
|
||
| # Test Service Ports | ||
|
|
||
| This workflow tests that the compiler automatically generates `--allow-host-service-ports` | ||
| from `services:` port mappings. | ||
|
|
||
| Expected: the compiled lock file includes `--allow-host-service-ports` with expressions for | ||
| both PostgreSQL (port 5432) and Redis (port 6379). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,8 +4,10 @@ import ( | |
| "encoding/json" | ||
| "fmt" | ||
| "maps" | ||
| "os" | ||
| "strings" | ||
|
|
||
| "github.com/github/gh-aw/pkg/console" | ||
| "github.com/github/gh-aw/pkg/logger" | ||
| "github.com/github/gh-aw/pkg/parser" | ||
| "github.com/goccy/go-yaml" | ||
|
|
@@ -529,6 +531,20 @@ func (c *Compiler) processAndMergeServices(frontmatter map[string]any, workflowD | |
| } | ||
| } | ||
| } | ||
|
|
||
| // Extract service port expressions for AWF --allow-host-service-ports | ||
| if workflowData.Services != "" { | ||
| expressions, warnings := ExtractServicePortExpressions(workflowData.Services) | ||
| workflowData.ServicePortExpressions = expressions | ||
| for _, w := range warnings { | ||
| orchestratorWorkflowLog.Printf("Warning: %s", w) | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(w)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Warnings emitted here should also call |
||
| c.IncrementWarningCount() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Smoke test review comment #2: Debug-only log here means users won't see the warning unless
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed — incrementing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Smoke test review comment #2: Debug-only log here means users won't see the warning unless
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed — incrementing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Smoke test review comment #2: Debug-only log here means users won't see the warning unless |
||
| } | ||
|
Comment on lines
+535
to
+543
|
||
| if expressions != "" { | ||
| orchestratorWorkflowLog.Printf("Extracted service port expressions: %s", expressions) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // mergeJobsFromYAMLImports merges jobs from imported YAML workflows with main workflow jobs | ||
|
|
||
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.
Warnings are emitted to stderr here but the compiler’s warning counter isn’t incremented. To keep summary/exit behavior consistent with other warnings (e.g. pkg/workflow/compiler.go), call c.IncrementWarningCount() for each emitted warning.
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.
Agreed — incrementing
c.IncrementWarningCount()is important for consistent warning summary behavior across the compiler. Good catch! 🔍