Skip to content

Commit 2fd4249

Browse files
CybotTMCybotTM
committed
fix: address PR review — add save/subject inheritance, fix doc key names
- 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 parent cb01554 commit 2fd4249

File tree

4 files changed

+95
-23
lines changed

4 files changed

+95
-23
lines changed

‎cli/config.go‎

Lines changed: 32 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -387,7 +387,7 @@ func (c *Config) registerAllJobs() {
387387
j.Provider = provider
388388
j.InitializeRuntimeFields()
389389
j.Name = name
390-
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig)
390+
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig, &j.SaveConfig)
391391
c.injectDedup(&j.SlackConfig, &j.MailConfig)
392392
j.buildMiddlewares(wm)
393393
_ = c.sh.AddJob(j)
@@ -401,15 +401,15 @@ func (c *Config) registerAllJobs() {
401401
j.Provider = provider
402402
j.InitializeRuntimeFields()
403403
j.Name = name
404-
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig)
404+
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig, &j.SaveConfig)
405405
c.injectDedup(&j.SlackConfig, &j.MailConfig)
406406
j.buildMiddlewares(wm)
407407
_ = c.sh.AddJob(j)
408408
}
409409
for name, j := range c.LocalJobs {
410410
_ = defaults.Set(j)
411411
j.Name = name
412-
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig)
412+
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig, &j.SaveConfig)
413413
c.injectDedup(&j.SlackConfig, &j.MailConfig)
414414
j.buildMiddlewares(wm)
415415
_ = c.sh.AddJob(j)
@@ -423,15 +423,15 @@ func (c *Config) registerAllJobs() {
423423
j.Provider = provider
424424
j.InitializeRuntimeFields()
425425
j.Name = name
426-
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig)
426+
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig, &j.SaveConfig)
427427
c.injectDedup(&j.SlackConfig, &j.MailConfig)
428428
j.buildMiddlewares(wm)
429429
_ = c.sh.AddJob(j)
430430
}
431431
for name, j := range c.ComposeJobs {
432432
_ = defaults.Set(j)
433433
j.Name = name
434-
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig)
434+
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig, &j.SaveConfig)
435435
c.injectDedup(&j.SlackConfig, &j.MailConfig)
436436
j.buildMiddlewares(wm)
437437
_ = c.sh.AddJob(j)
@@ -450,9 +450,10 @@ func (c *Config) injectDedup(slack *middlewares.SlackConfig, mail *middlewares.M
450450
// mergeNotificationDefaults copies global notification settings to job-level configs
451451
// when the job-level field has its zero value. This allows partial overrides where
452452
// a job can specify only `mail-only-on-error: true` while inheriting SMTP settings.
453-
func (c *Config) mergeNotificationDefaults(slack *middlewares.SlackConfig, mail *middlewares.MailConfig) {
453+
func (c *Config) mergeNotificationDefaults(slack *middlewares.SlackConfig, mail *middlewares.MailConfig, save *middlewares.SaveConfig) {
454454
c.mergeSlackDefaults(slack)
455455
c.mergeMailDefaults(mail)
456+
c.mergeSaveDefaults(save)
456457
}
457458

458459
// mergeSlackDefaults copies global Slack settings to job config where job has zero values
@@ -501,12 +502,27 @@ func (c *Config) mergeMailDefaults(job *middlewares.MailConfig) {
501502
if job.EmailFrom == "" {
502503
job.EmailFrom = global.EmailFrom
503504
}
505+
if job.EmailSubject == "" {
506+
job.EmailSubject = global.EmailSubject
507+
}
504508
// MailOnlyOnError: inherit from global only when the job didn't explicitly set it (nil).
505509
if job.MailOnlyOnError == nil && global.MailOnlyOnError != nil {
506510
job.MailOnlyOnError = middlewares.BoolPtr(*global.MailOnlyOnError)
507511
}
508512
}
509513

514+
// mergeSaveDefaults copies global Save settings to job config where job has zero values
515+
func (c *Config) mergeSaveDefaults(job *middlewares.SaveConfig) {
516+
global := &c.Global.SaveConfig
517+
if job.SaveFolder == "" {
518+
job.SaveFolder = global.SaveFolder
519+
}
520+
// SaveOnlyOnError: inherit from global only when the job didn't explicitly set it (nil).
521+
if job.SaveOnlyOnError == nil && global.SaveOnlyOnError != nil {
522+
job.SaveOnlyOnError = middlewares.BoolPtr(*global.SaveOnlyOnError)
523+
}
524+
}
525+
510526
// UserContainerDefault is the sentinel value that explicitly requests the container's default user,
511527
// overriding any global default-user setting.
512528
const UserContainerDefault = "default"
@@ -658,7 +674,7 @@ func (c *Config) dockerLabelsUpdate(labels map[string]map[string]string) {
658674
j.Provider = c.dockerHandler.GetDockerProvider()
659675
j.InitializeRuntimeFields()
660676
j.Name = name
661-
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig)
677+
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig, &j.SaveConfig)
662678
c.injectDedup(&j.SlackConfig, &j.MailConfig)
663679
}
664680
syncJobMap(c, c.ExecJobs, parsedLabelConfig.ExecJobs, execPrep, JobSourceLabel, "exec")
@@ -672,15 +688,15 @@ func (c *Config) dockerLabelsUpdate(labels map[string]map[string]string) {
672688
j.Provider = c.dockerHandler.GetDockerProvider()
673689
j.InitializeRuntimeFields()
674690
j.Name = name
675-
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig)
691+
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig, &j.SaveConfig)
676692
c.injectDedup(&j.SlackConfig, &j.MailConfig)
677693
}
678694
syncJobMap(c, c.RunJobs, parsedLabelConfig.RunJobs, runPrep, JobSourceLabel, "run")
679695

680696
localPrep := func(name string, j *LocalJobConfig) {
681697
_ = defaults.Set(j)
682698
j.Name = name
683-
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig)
699+
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig, &j.SaveConfig)
684700
c.injectDedup(&j.SlackConfig, &j.MailConfig)
685701
}
686702

@@ -693,14 +709,14 @@ func (c *Config) dockerLabelsUpdate(labels map[string]map[string]string) {
693709
j.Provider = c.dockerHandler.GetDockerProvider()
694710
j.InitializeRuntimeFields()
695711
j.Name = name
696-
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig)
712+
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig, &j.SaveConfig)
697713
c.injectDedup(&j.SlackConfig, &j.MailConfig)
698714
}
699715

700716
composePrep := func(name string, j *ComposeJobConfig) {
701717
_ = defaults.Set(j)
702718
j.Name = name
703-
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig)
719+
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig, &j.SaveConfig)
704720
c.injectDedup(&j.SlackConfig, &j.MailConfig)
705721
}
706722

@@ -785,7 +801,7 @@ func (c *Config) iniConfigUpdate() error {
785801
j.Provider = c.dockerHandler.GetDockerProvider()
786802
j.InitializeRuntimeFields()
787803
j.Name = name
788-
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig)
804+
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig, &j.SaveConfig)
789805
c.injectDedup(&j.SlackConfig, &j.MailConfig)
790806
}
791807
syncJobMap(c, c.ExecJobs, parsed.ExecJobs, execPrep, JobSourceINI, "exec")
@@ -799,15 +815,15 @@ func (c *Config) iniConfigUpdate() error {
799815
j.Provider = c.dockerHandler.GetDockerProvider()
800816
j.InitializeRuntimeFields()
801817
j.Name = name
802-
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig)
818+
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig, &j.SaveConfig)
803819
c.injectDedup(&j.SlackConfig, &j.MailConfig)
804820
}
805821
syncJobMap(c, c.RunJobs, parsed.RunJobs, runPrep, JobSourceINI, "run")
806822

807823
localPrep := func(name string, j *LocalJobConfig) {
808824
_ = defaults.Set(j)
809825
j.Name = name
810-
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig)
826+
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig, &j.SaveConfig)
811827
c.injectDedup(&j.SlackConfig, &j.MailConfig)
812828
}
813829
syncJobMap(c, c.LocalJobs, parsed.LocalJobs, localPrep, JobSourceINI, "local")
@@ -821,15 +837,15 @@ func (c *Config) iniConfigUpdate() error {
821837
j.Provider = c.dockerHandler.GetDockerProvider()
822838
j.InitializeRuntimeFields()
823839
j.Name = name
824-
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig)
840+
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig, &j.SaveConfig)
825841
c.injectDedup(&j.SlackConfig, &j.MailConfig)
826842
}
827843
syncJobMap(c, c.ServiceJobs, parsed.ServiceJobs, svcPrep, JobSourceINI, "service")
828844

829845
composePrep := func(name string, j *ComposeJobConfig) {
830846
_ = defaults.Set(j)
831847
j.Name = name
832-
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig)
848+
c.mergeNotificationDefaults(&j.SlackConfig, &j.MailConfig, &j.SaveConfig)
833849
c.injectDedup(&j.SlackConfig, &j.MailConfig)
834850
}
835851
syncJobMap(c, c.ComposeJobs, parsed.ComposeJobs, composePrep, JobSourceINI, "compose")

‎cli/config_test.go‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -842,3 +842,58 @@ func TestMergeMailDefaultsOnlyOnErrorInheritance(t *testing.T) {
842842
require.NotNil(t, jobMail5.MailOnlyOnError)
843843
assert.True(t, *jobMail5.MailOnlyOnError, "Both true - error-only setting should be preserved")
844844
}
845+
846+
func TestMergeMailDefaultsEmailSubjectInheritance(t *testing.T) {
847+
t.Parallel()
848+
849+
cfg := NewConfig(&TestLogger{})
850+
cfg.Global.MailConfig.EmailSubject = "[{{status .Execution}}] {{.Job.GetName}}"
851+
852+
// Job without subject inherits from global
853+
jobMail := middlewares.MailConfig{SMTPHost: "mail.example.com"}
854+
cfg.mergeMailDefaults(&jobMail)
855+
assert.Equal(t, "[{{status .Execution}}] {{.Job.GetName}}", jobMail.EmailSubject)
856+
857+
// Job with explicit subject keeps its own
858+
jobMail2 := middlewares.MailConfig{
859+
SMTPHost: "mail.example.com",
860+
EmailSubject: "Custom: {{.Job.GetName}}",
861+
}
862+
cfg.mergeMailDefaults(&jobMail2)
863+
assert.Equal(t, "Custom: {{.Job.GetName}}", jobMail2.EmailSubject)
864+
}
865+
866+
func TestMergeSaveDefaults(t *testing.T) {
867+
t.Parallel()
868+
869+
cfg := NewConfig(&TestLogger{})
870+
cfg.Global.SaveConfig.SaveFolder = "/var/log/ofelia"
871+
cfg.Global.SaveConfig.SaveOnlyOnError = middlewares.BoolPtr(true)
872+
873+
// Job without save settings inherits from global
874+
jobSave := middlewares.SaveConfig{}
875+
cfg.mergeSaveDefaults(&jobSave)
876+
assert.Equal(t, "/var/log/ofelia", jobSave.SaveFolder)
877+
require.NotNil(t, jobSave.SaveOnlyOnError, "Global save-only-on-error=true should propagate to job")
878+
assert.True(t, *jobSave.SaveOnlyOnError)
879+
880+
// Job with explicit folder keeps its own
881+
jobSave2 := middlewares.SaveConfig{SaveFolder: "/custom/logs"}
882+
cfg.mergeSaveDefaults(&jobSave2)
883+
assert.Equal(t, "/custom/logs", jobSave2.SaveFolder)
884+
require.NotNil(t, jobSave2.SaveOnlyOnError)
885+
assert.True(t, *jobSave2.SaveOnlyOnError, "SaveOnlyOnError should still inherit")
886+
887+
// Job can explicitly override to false
888+
jobSave3 := middlewares.SaveConfig{SaveOnlyOnError: middlewares.BoolPtr(false)}
889+
cfg.mergeSaveDefaults(&jobSave3)
890+
require.NotNil(t, jobSave3.SaveOnlyOnError)
891+
assert.False(t, *jobSave3.SaveOnlyOnError, "Job explicit false should NOT be overridden by global true")
892+
893+
// Empty global: nothing inherited
894+
cfgEmpty := NewConfig(&TestLogger{})
895+
jobSave4 := middlewares.SaveConfig{}
896+
cfgEmpty.mergeSaveDefaults(&jobSave4)
897+
assert.Empty(t, jobSave4.SaveFolder)
898+
assert.Nil(t, jobSave4.SaveOnlyOnError)
899+
}

‎docs/CONFIGURATION.md‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -632,7 +632,7 @@ command = critical-check.sh
632632
email-to = ops@example.com,alerts@example.com
633633
email-subject = Critical Job Report
634634
email-from = ofelia@example.com
635-
email-only-on-error = true
635+
mail-only-on-error = true
636636
```
637637

638638
### Configuration Inheritance
@@ -643,8 +643,9 @@ Notification settings support inheritance from global to job-level configuration
643643

644644
| Setting Type | Inherited Fields | Notes |
645645
|--------------|------------------|-------|
646-
| **Email** | `smtp-host`, `smtp-port`, `smtp-user`, `smtp-password`, `email-from`, `email-to` | SMTP connection details are inherited |
647-
| **Slack** | `slack-webhook` | Webhook URL is inherited |
646+
| **Email** | `smtp-host`, `smtp-port`, `smtp-user`, `smtp-password`, `email-from`, `email-to`, `email-subject`, `mail-only-on-error` | SMTP connection details and behavior flags are inherited |
647+
| **Slack** | `slack-webhook`, `slack-only-on-error` | Webhook URL and behavior flags are inherited |
648+
| **Save** | `save-folder`, `save-only-on-error` | Save folder and behavior flags are inherited |
648649

649650
**Example: Partial Override**
650651

@@ -663,7 +664,7 @@ schedule = @daily
663664
container = postgres
664665
command = pg_dump mydb > /backup/db.sql
665666
# Only override error-only behavior - inherits all SMTP settings from global
666-
email-only-on-error = true
667+
mail-only-on-error = true
667668
668669
[job-exec "critical-check"]
669670
schedule = @hourly
@@ -675,7 +676,7 @@ email-to = critical-alerts@example.com
675676

676677
**Important Notes:**
677678

678-
- Boolean fields (`email-only-on-error`, `slack-only-on-error`, `save-only-on-error`) are fully inherited from global config and can be overridden per-job in both directions (global true + job false, or global false + job true)
679+
- Boolean fields (`mail-only-on-error`, `slack-only-on-error`, `save-only-on-error`) are fully inherited from global config and can be overridden per-job in both directions (global true + job false, or global false + job true)
679680
- Job-level settings always take precedence over global settings when explicitly set
680681
- To enable notifications for a job, at minimum specify `email-to` or `slack-webhook` at either global or job level
681682

‎docs/QUICK_REFERENCE.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -598,7 +598,7 @@ email-to = admin@example.com
598598
schedule = @hourly
599599
container = app
600600
command = /critical-job.sh
601-
email-only-on-error = false # Get success notifications too
601+
mail-only-on-error = false # Get success notifications too
602602
```
603603

604604
### Both Slack and Email
@@ -615,7 +615,7 @@ slack-only-on-error = true
615615

616616
# Email for detailed logs
617617
email-to = team@example.com
618-
email-only-on-error = false
618+
mail-only-on-error = false
619619
```
620620

621621
## Security Checklist

0 commit comments

Comments
 (0)