fix(config): inherit *-only-on-error settings from global to job configs #455
Conversation
Change MailOnlyOnError, SlackOnlyOnError, and SaveOnlyOnError from bool to *bool so that "not set" (nil) is distinguishable from "explicitly false". This enables full bidirectional inheritance: - Global true + Job not set → Job inherits true - Global false + Job true → Job keeps true - Global true + Job false → Job keeps false (explicit override) Previously, inheriting SMTP/Slack settings from global caused job-level middleware to be created with OnlyOnError=false (Go zero value), which silently overrode the global middleware (dedup by type) and sent notifications on every execution — even when global set only-on-error. Also add mergeNotificationDefaults calls to Docker label and INI config update paths, which were missing notification config inheritance. Fixes #454
Summary of ChangesHello @CybotTM, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request provides a crucial fix for how notification-related boolean flags, specifically Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
✅ Mutation Testing ResultsMutation Score: 100.00% (threshold: 60%)
What is mutation testing?Mutation testing measures test quality by introducing small changes (mutations) to the code and checking if tests detect them. A higher score means better test effectiveness.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a bug in how *-only-on-error settings are inherited from the global configuration to job-specific configurations by changing boolean flags to pointers. However, a critical vulnerability was identified: while SaveOnlyOnError was also changed to a pointer type, the corresponding inheritance logic in the configuration loader was omitted. This oversight results in the global save-only-on-error setting being ignored for individual jobs, potentially leading to more logs being saved to disk than intended. Additionally, a minor refactoring in cli/config.go is suggested to reduce code duplication in job preparation functions, which would improve long-term maintainability.
WalkthroughThis PR refactors boolean configuration fields ( Changes
Sequence DiagramThis diagram shows the interactions between components: sequenceDiagram
participant Config as Configuration System
participant JobPrep as Job Preparation Functions
participant MergeDefaults as mergeNotificationDefaults()
participant InjectDedup as injectDedup()
participant SlackConfig as SlackConfig
participant MailConfig as MailConfig
Note over Config,MailConfig: Job Configuration Flow (All Job Types)
Config->>JobPrep: Initialize Job (exec/run/local/service/compose)
activate JobPrep
JobPrep->>JobPrep: Set defaults
JobPrep->>JobPrep: Set Provider & RuntimeFields
JobPrep->>JobPrep: Set Name
Note over JobPrep,InjectDedup: New: Notification Configuration
JobPrep->>MergeDefaults: mergeNotificationDefaults(&SlackConfig, &MailConfig)
activate MergeDefaults
MergeDefaults->>SlackConfig: Check SlackWebhook
alt SlackWebhook is empty
MergeDefaults->>SlackConfig: Inherit global.SlackWebhook
end
MergeDefaults->>SlackConfig: Check SlackOnlyOnError
alt job.SlackOnlyOnError == nil AND global.SlackOnlyOnError != nil
MergeDefaults->>SlackConfig: Inherit global.SlackOnlyOnError (via BoolPtr)
end
MergeDefaults->>MailConfig: Check EmailFrom
alt EmailFrom is empty
MergeDefaults->>MailConfig: Inherit global.EmailFrom
end
MergeDefaults->>MailConfig: Check MailOnlyOnError
alt job.MailOnlyOnError == nil AND global.MailOnlyOnError != nil
MergeDefaults->>MailConfig: Inherit global.MailOnlyOnError (via BoolPtr)
end
MergeDefaults-->>JobPrep: Defaults merged
deactivate MergeDefaults
JobPrep->>InjectDedup: injectDedup(&SlackConfig, &MailConfig)
activate InjectDedup
InjectDedup->>SlackConfig: Configure deduplication
InjectDedup->>MailConfig: Configure deduplication
InjectDedup-->>JobPrep: Dedup configured
deactivate InjectDedup
JobPrep-->>Config: Job fully configured
deactivate JobPrep
Note over Config,MailConfig: Pattern applies to:<br/>- ExecJobs (Label & INI)<br/>- RunJobs (Label & INI)<br/>- LocalJobs (Label & INI)<br/>- ServiceJobs (Label & INI)<br/>- ComposeJobs (Label & INI)
🔗 Cross-Repository Impact AnalysisEnable automatic detection of breaking changes across your dependent repositories. → Set up now Learn more about Cross-Repository AnalysisWhat It Does
How to Enable
Benefits
Note for WindsurfPlease change the default marketplace provider to the following in the windsurf settings:Marketplace Extension Gallery Service URL: https://marketplace.visualstudio.com/_apis/public/gallery Marketplace Gallery Item URL: https://marketplace.visualstudio.com/items Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts belowEmoji Descriptions:
Interact with the Bot:
Also you can trigger various commands with the bot by doing The current supported commands are
More commands to be added soon. |
There was a problem hiding this comment.
Pull request overview
Fixes notification “only-on-error” inheritance so global settings correctly propagate to per-job middleware configs, addressing a regression where job-level middlewares could suppress global behavior due to middleware type deduplication.
Changes:
- Change
MailOnlyOnError,SlackOnlyOnError,SaveOnlyOnErrorfromboolto*booland add safe helpers for nil handling. - Update notification middlewares to interpret
nilas default “send/save on all executions”, and update tests accordingly. - Ensure config update paths (Docker labels + INI reload) call notification-default merging and dedup injection for jobs.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
cli/config.go |
Implements inheritance of *-only-on-error from global to job configs; ensures merge/inject is applied on label + INI update paths. |
cli/config_test.go |
Adds/updates unit tests validating inheritance behavior across nil/true/false combinations. |
cli/config_extra_test.go |
Updates INI reload test assertions for pointer-backed booleans. |
middlewares/common.go |
Adds boolVal(*bool) and BoolPtr(bool) helpers to support tri-state booleans. |
middlewares/mail.go |
Switches MailOnlyOnError to *bool and updates runtime logic to handle nil safely. |
middlewares/slack.go |
Switches SlackOnlyOnError to *bool and updates runtime logic to handle nil safely. |
middlewares/save.go |
Switches SaveOnlyOnError to *bool and updates runtime logic to handle nil safely. |
middlewares/slack_test.go |
Updates Slack middleware test to use BoolPtr(true). |
middlewares/save_test.go |
Updates Save middleware test to use BoolPtr(true). |
docs/packages/middlewares.md |
Updates docs/examples to reflect *bool fields and BoolPtr(...) usage. |
docs/CONFIGURATION.md |
Updates inheritance documentation for boolean notification fields. |
- Add mergeSaveDefaults() to inherit save-folder and save-only-on-error from global config to job configs (was missing after *bool change) - Add EmailSubject inheritance in mergeMailDefaults() - Fix docs using wrong config key: email-only-on-error → mail-only-on-error - Update inheritance table to include Save and all inherited fields - Add tests for mergeSaveDefaults and EmailSubject inheritance
1 participant
Summary
MailOnlyOnError,SlackOnlyOnError,SaveOnlyOnErrorfromboolto*boolso "not set" (nil) is distinguishable from "explicitlyfalse"*-only-on-errorfrom global config when job doesn't set it, while allowing explicit per-job overrides in both directionsmergeNotificationDefaultscalls to Docker label and INI config update pathsRoot cause
Commit 4218a6e introduced SMTP/Slack setting inheritance from global to job configs. This made job-level
MailConfignon-empty, causing a job-level Mail middleware to be created that silently replaced the global one (middlewares deduplicate by type). Butmail-only-on-errorwas not inherited (Go bool zero-value problem), so the job-level middleware defaulted tofalseand sent emails on every execution.Inheritance matrix
truetrue(inherited)falsetruetrue(job override)truefalsefalse(job override)Test plan
go test ./...)Fixes #454