-
Notifications
You must be signed in to change notification settings - Fork 373
Remove observability.job-summary opt-in, render job summary when OTLP is enabled #24750
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
2c1af6c
4ca6702
3256ac0
9b64fa2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,12 +114,6 @@ function buildObservabilitySummary(data) { | |
| } | ||
|
|
||
| async function main(core) { | ||
| const mode = process.env.GH_AW_OBSERVABILITY_JOB_SUMMARY || ""; | ||
| if (mode !== "on") { | ||
| core.info(`Skipping observability summary: mode=${mode || "unset"}`); | ||
| return; | ||
| } | ||
|
|
||
| const data = collectObservabilityData(); | ||
| const markdown = buildObservabilitySummary(data); | ||
| await core.summary.addRaw(markdown).write(); | ||
|
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. Nice simplification — removing the |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1646,14 +1646,7 @@ | |
| "observability": { | ||
| "type": "object", | ||
| "desc": "Optional observability output settings for workflow runs.", | ||
| "children": { | ||
| "job-summary": { | ||
| "type": "string", | ||
| "desc": "If set to 'on', append a compact observability section to the GitHub Actions job summary.", | ||
| "enum": ["on", "off"], | ||
| "leaf": true | ||
| } | ||
| } | ||
| "children": {} | ||
| }, | ||
|
Comment on lines
1646
to
1650
|
||
| "bots": { | ||
| "type": "array", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5905,11 +5905,7 @@ secret-masking: | |
|
|
||
| # Optional observability output settings for workflow runs. | ||
| # (optional) | ||
| observability: | ||
| # If set to 'on', append a compact observability section to the GitHub Actions job | ||
| # summary. Defaults to off when omitted. | ||
| # (optional) | ||
| job-summary: "on" | ||
| observability: {} | ||
|
|
||
|
Comment on lines
5906
to
5909
|
||
| # Allow list of bot identifiers that can trigger the workflow even if they don't | ||
| # meet the required role permissions. When the actor is in this list, the bot must | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ type importAccumulator struct { | |
| secretMaskingBuilder strings.Builder | ||
| postStepsBuilder strings.Builder | ||
| jobsBuilder strings.Builder // Jobs from imported YAML workflows | ||
| observabilityBuilder strings.Builder // observability config (first-wins for OTLP endpoint) | ||
| engines []string | ||
| safeOutputs []string | ||
| mcpScripts []string | ||
|
|
@@ -350,6 +351,17 @@ func (acc *importAccumulator) extractAllImportFields(content []byte, item import | |
| } | ||
| } | ||
|
|
||
| // Extract observability from imported file (first-wins: only set if not yet populated). | ||
| // This ensures an imported workflow's OTLP config is visible to injectOTLPConfig even | ||
| // when the main workflow's frontmatter does not contain an observability section. | ||
| if acc.observabilityBuilder.Len() == 0 { | ||
|
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. The new |
||
| obsContent, obsErr := extractFieldJSONFromMap(fm, "observability", "{}") | ||
| if obsErr == nil && obsContent != "" && obsContent != "{}" { | ||
| acc.observabilityBuilder.WriteString(obsContent) | ||
| log.Printf("Extracted observability from import: %s", item.fullPath) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
|
|
@@ -381,6 +393,7 @@ func (acc *importAccumulator) toImportsResult(topologicalOrder []string) *Import | |
| MergedCaches: acc.caches, | ||
| MergedJobs: acc.jobsBuilder.String(), | ||
| MergedFeatures: acc.features, | ||
| MergedObservability: acc.observabilityBuilder.String(), | ||
| ImportedFiles: topologicalOrder, | ||
| AgentFile: acc.agentFile, | ||
| AgentImportSpec: acc.agentImportSpec, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -248,7 +248,9 @@ func TestParseFrontmatterConfig(t *testing.T) { | |
| t.Run("handles observability configuration", func(t *testing.T) { | ||
| frontmatter := map[string]any{ | ||
| "observability": map[string]any{ | ||
| "job-summary": "on", | ||
| "otlp": map[string]any{ | ||
| "endpoint": "https://traces.example.com", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
|
|
@@ -261,8 +263,8 @@ func TestParseFrontmatterConfig(t *testing.T) { | |
| t.Fatal("Observability should not be nil") | ||
| } | ||
|
|
||
| if config.Observability.JobSummary != "on" { | ||
| t.Errorf("JobSummary = %q, want %q", config.Observability.JobSummary, "on") | ||
| if config.Observability.OTLP == nil { | ||
| t.Fatal("OTLP should not be nil") | ||
| } | ||
|
Comment on lines
248
to
268
|
||
| }) | ||
|
|
||
|
|
||
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.
Good simplification — removing the
GH_AW_OBSERVABILITY_JOB_SUMMARYmode gate makes the behavior cleaner. The summary is now always generated when called, which aligns with the new OTLP-gated invocation in the compiler.