Conversation
jenn-if-err
commented
Mar 12, 2026
jenn-if-err
commented
- add nil guards for metadata
- remove the run summary tracker
There was a problem hiding this comment.
Pull request overview
This PR updates the distributed scenario runner to be more resilient to missing metadata, removes the in-process run summary tracker, and expands the completion payload/notifications with additional fields (failed scenarios + test-analysis flags).
Changes:
- Add nil-guards for
metadatain scenario distribution paths and propagatetotal_scenariosmore consistently. - Remove
runSummary/runTrackerand the per-scenario completion callback previously used to emit a final Slack summary. - Extend completion payload handling (and GitHub repository_dispatch payload) with
failed_scenarios,missing_tests_in_pr, andshould_run_tests, and adjust Slack completion formatting.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
scenario.go |
Adds extra metadata attributes (including nested test_analysis flags) to Pub/Sub report attributes. |
main.go |
Adds metadata nil guards in distributors; removes run summary tracking; updates completion Slack formatting and message struct fields. |
github.go |
Adds new fields to the repository_dispatch client payload and logs them. |
go.mod |
Promotes Secret Manager dependency to a direct requirement (matches secret.go usage). |
go.sum |
Tidies dependency hashes/versions accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log.Printf("repository_dispatch sent: repo=%s sha=%s run_id=%s overall_status=%s", | ||
| msg.Repository, msg.CommitSHA, msg.RunID, msg.OverallStatus) | ||
| log.Printf("repository_dispatch sent: repo=%s sha=%s run_id=%s overall_status=%s missing_tests_in_pr=%v should_run_tests=%v", | ||
| msg.Repository, msg.CommitSHA, msg.RunID, msg.OverallStatus, msg.MissingTestsInPR, msg.ShouldRunTests) |
There was a problem hiding this comment.
The continuation line for this log.Printf call is indented with spaces instead of gofmt's tab indentation. Please run gofmt on the file to keep formatting consistent.
| msg.Repository, msg.CommitSHA, msg.RunID, msg.OverallStatus, msg.MissingTestsInPR, msg.ShouldRunTests) | |
| msg.Repository, msg.CommitSHA, msg.RunID, msg.OverallStatus, msg.MissingTestsInPR, msg.ShouldRunTests) |
| } | ||
| } | ||
|
|
||
| if scenariopubsub != "" && githubtoken != "" && pubsub != "" { |
There was a problem hiding this comment.
@Ynah537 can I remove this? Start_all doesn't have githubtoken, if this is here then the code couldn't proceed.
There was a problem hiding this comment.
It will still be safe since there is a githubtoken checker in the sendrepositorydispatch
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
github.go:1
- Adding
missing_tests_in_prandshould_run_testsinto amap[string]interface{}will always include these keys (even whenfalse), which differs from theomitemptybehavior inScenarioProgressMessageand can be a breaking change for consumers that treat absence differently fromfalse. If you want to preserve optional semantics, only add these keys when they are meaningful (e.g., conditionally whentrueor when the upstream analysis exists), or switchclient_payloadto a typed struct withomitempty.
package main
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ta, ok := in.Metadata["test_analysis"].(map[string]interface{}); ok { | ||
| for _, key := range []string{"missing_tests_in_pr", "should_run_tests"} { | ||
| if v, ok := ta[key].(bool); ok { | ||
| attr[key] = fmt.Sprintf("%v", v) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Converting booleans via fmt.Sprintf("%v", v) is heavier than needed and can allocate; strconv.FormatBool(v) is the idiomatic and more efficient conversion for booleans.
| } | ||
|
|
||
| if scenariopubsub != "" && githubtoken != "" && pubsub != "" { | ||
| if scenariopubsub != "" && pubsub != "" { |
There was a problem hiding this comment.
The progress listener now starts even when githubtoken is empty, and the previous warning about missing token was removed. If GitHub repository_dispatch is still attempted within the listener path, this change can lead to repeated failing calls (401) with reduced operator visibility. Consider restoring a warning when GitHub dispatch is configured but token is missing, or ensuring the dispatch call is explicitly skipped (with a clear log) when githubtoken == "".
| if scenariopubsub != "" && pubsub != "" { | |
| if scenariopubsub != "" && pubsub != "" { | |
| if githubtoken == "" { | |
| log.Printf("WARNING: githubtoken is empty; scenario progress listener will run, but any GitHub repository_dispatch operations may fail due to missing credentials") | |
| } |
| }() | ||
| } else if scenariopubsub != "" && githubtoken == "" { | ||
| log.Printf("WARNING: --scenario-pubsub set but github token is empty; set --secret-project-id or --github-token to enable GitHub status updates") | ||
| } |
There was a problem hiding this comment.
The progress listener now starts even when githubtoken is empty, and the previous warning about missing token was removed. If GitHub repository_dispatch is still attempted within the listener path, this change can lead to repeated failing calls (401) with reduced operator visibility. Consider restoring a warning when GitHub dispatch is configured but token is missing, or ensuring the dispatch call is explicitly skipped (with a clear log) when githubtoken == "".
