Don't count diff2.REPORT (informational warnings) as a "correction" (#2361)

Co-authored-by: Tom Limoncelli <tal@whatexit.org>

commands/commands.go

@@ -26,8 +26,6 @@ var commands = []*cli.Command{}
 // These are set by/for goreleaser
 var (
 	version = "dev"
-	commit  = "none"
-	date    = "unknown"
 )
 
 func cmd(cat string, c *cli.Command) bool {

commands/previewPush.go

@@ -222,13 +222,16 @@ func run(args PreviewArgs, push bool, interactive bool, out printer.CLI) error {
 					continue
 				}
 
-				corrections, err := zonerecs.CorrectZoneRecords(provider.Driver, domain)
+				reports, corrections, err := zonerecs.CorrectZoneRecords(provider.Driver, domain)
+				printReports(domain.Name, provider.Name, reports, out, push, notifier)
 				out.EndProvider(provider.Name, len(corrections), err)
 				if err != nil {
 					anyErrors = true
 					return
 				}
 				totalCorrections += len(corrections)
+				// When diff1 goes away, the call to printReports() should be moved to HERE.
+				//printReports(domain.Name, provider.Name, reports, out, push, notifier)
 				anyErrors = printOrRunCorrections(domain.Name, provider.Name, corrections, out, push, interactive, notifier) || anyErrors
 			}
 
@@ -543,3 +546,15 @@ func printOrRunCorrections(domain string, provider string, corrections []*models
 	}
 	return anyErrors
 }
+
+func printReports(domain string, provider string, reports []*models.Correction, out printer.CLI, push bool, notifier notifications.Notifier) (anyErrors bool) {
+	anyErrors = false
+	if len(reports) == 0 {
+		return false
+	}
+	for i, report := range reports {
+		out.PrintReport(i, report)
+		notifier.Notify(domain, provider, report.Msg, nil, !push)
+	}
+	return anyErrors
+}

integrationTest/integration_test.go

@@ -223,12 +223,12 @@ func makeChanges(t *testing.T, prv providers.DNSServiceProvider, dc *models.Doma
 		}
 
 		// get and run corrections for first time
-		corrections, err := zonerecs.CorrectZoneRecords(prv, dom)
+		_, corrections, err := zonerecs.CorrectZoneRecords(prv, dom)
 		if err != nil {
 			t.Fatal(fmt.Errorf("runTests: %w", err))
 		}
 		if tst.Changeless {
-			if count := zonerecs.CountActionable(corrections); count != 0 {
+			if count := len(corrections); count != 0 {
 				t.Logf("Expected 0 corrections on FIRST run, but found %d.", count)
 				for i, c := range corrections {
 					t.Logf("UNEXPECTED #%d: %s", i, c.Msg)
@@ -256,11 +256,11 @@ func makeChanges(t *testing.T, prv providers.DNSServiceProvider, dc *models.Doma
 		}
 
 		// run a second time and expect zero corrections
-		corrections, err = zonerecs.CorrectZoneRecords(prv, dom2)
+		_, corrections, err = zonerecs.CorrectZoneRecords(prv, dom2)
 		if err != nil {
 			t.Fatal(err)
 		}
-		if count := zonerecs.CountActionable(corrections); count != 0 {
+		if count := len(corrections); count != 0 {
 			t.Logf("Expected 0 corrections on second run, but found %d.", count)
 			for i, c := range corrections {
 				t.Logf("UNEXPECTED #%d: %s", i, c.Msg)
@@ -351,10 +351,13 @@ func TestDualProviders(t *testing.T) {
 	run := func() {
 		dom, _ := dc.Copy()
 
-		cs, err := zonerecs.CorrectZoneRecords(p, dom)
+		rs, cs, err := zonerecs.CorrectZoneRecords(p, dom)
 		if err != nil {
 			t.Fatal(err)
 		}
+		for i, c := range rs {
+			t.Logf("INFO#%d:\n%s", i+1, c.Msg)
+		}
 		for i, c := range cs {
 			t.Logf("#%d:\n%s", i+1, c.Msg)
 			if err = c.F(); err != nil {
@@ -373,14 +376,17 @@ func TestDualProviders(t *testing.T) {
 	run()
 	// run again to make sure no corrections
 	t.Log("Running again to ensure stability")
-	cs, err := zonerecs.CorrectZoneRecords(p, dc)
+	rs, cs, err := zonerecs.CorrectZoneRecords(p, dc)
 	if err != nil {
 		t.Fatal(err)
 	}
-	if count := zonerecs.CountActionable(cs); count != 0 {
+	if count := len(cs); count != 0 {
 		t.Logf("Expect no corrections on second run, but found %d.", count)
+		for i, c := range rs {
+			t.Logf("INFO#%d:\n%s", i+1, c.Msg)
+		}
 		for i, c := range cs {
-			t.Logf("#%d: %s", i, c.Msg)
+			t.Logf("#%d: %s", i+1, c.Msg)
 		}
 		t.FailNow()
 	}

models/domain.go

@@ -9,8 +9,10 @@ import (
 )
 
 const (
+	// DomainUniqueName is the full `example.com!tag` name`
 	DomainUniqueName = "dnscontrol_uniquename"
-	DomainTag        = "dnscontrol_tag"
+	// DomainTag is the tag part of `example.com!tag` name
+	DomainTag = "dnscontrol_tag"
 )
 
 // DomainConfig describes a DNS domain (technically a DNS zone).

pkg/acme/acme.go

@@ -263,7 +263,7 @@ func (c *certManager) ensureNoPendingCorrections(d *models.DomainConfig) error {
 	return nil
 }
 
-// IgnoredProviders is a lit of provider names that should not be used to fill challenges.
+// IgnoredProviders is a list of provider names that should not be used to fill challenges.
 var IgnoredProviders = map[string]bool{}
 
 func (c *certManager) getCorrections(d *models.DomainConfig) ([]*models.Correction, error) {
@@ -276,10 +276,13 @@ func (c *certManager) getCorrections(d *models.DomainConfig) ([]*models.Correcti
 		if err != nil {
 			return nil, err
 		}
-		corrections, err := zonerecs.CorrectZoneRecords(p.Driver, dc)
+		reports, corrections, err := zonerecs.CorrectZoneRecords(p.Driver, dc)
 		if err != nil {
 			return nil, err
 		}
+		for _, c := range reports {
+			c.Msg = fmt.Sprintf("INFO[%s] %s", p.Name, strings.TrimSpace(c.Msg))
+		}
 		for _, c := range corrections {
 			c.Msg = fmt.Sprintf("[%s] %s", p.Name, strings.TrimSpace(c.Msg))
 		}

pkg/diff/diff2compat.go

@@ -62,7 +62,13 @@ func (d *differCompat) IncrementalDiff(existing []*models.RecordConfig) (unchang
 		case diff2.REPORT:
 			// Sadly the NewCompat function doesn't have an equivalent. We
 			// just output the messages now.
+			fmt.Print("INFO: ")
 			fmt.Println(inst.MsgsJoined)
+
+			// TODO(tlim): When diff1 is deleted, IncremtntalDiff should add a
+			// parameter to list the REPORT messages. It can also eliminate the
+			// first parameter (existing) since nobody uses that in the diff2
+			// world.
 		case diff2.CREATE:
 			cor.Desired = inst.New[0]
 			toCreate = append(toCreate, cor)

pkg/diff2/handsoff.go

@@ -119,18 +119,18 @@ func handsoff(
 	// Process UNMANAGE/IGNORE_* and NO_PURGE features:
 	ignorable, foreign := processIgnoreAndNoPurge(domain, existing, desired, absences, unmanagedConfigs, noPurge)
 	if len(foreign) != 0 {
-		msgs = append(msgs, fmt.Sprintf("INFO: %d records not being deleted because of NO_PURGE:", len(foreign)))
+		msgs = append(msgs, fmt.Sprintf("%d records not being deleted because of NO_PURGE:", len(foreign)))
 		msgs = append(msgs, reportSkips(foreign, !printer.SkinnyReport)...)
 	}
 	if len(ignorable) != 0 {
-		msgs = append(msgs, fmt.Sprintf("INFO: %d records not being deleted because of IGNORE*():", len(ignorable)))
+		msgs = append(msgs, fmt.Sprintf("%d records not being deleted because of IGNORE*():", len(ignorable)))
 		msgs = append(msgs, reportSkips(ignorable, !printer.SkinnyReport)...)
 	}
 
 	// Check for invalid use of IGNORE_*.
 	conflicts := findConflicts(unmanagedConfigs, desired)
 	if len(conflicts) != 0 {
-		msgs = append(msgs, fmt.Sprintf("INFO: %d records that are both IGNORE*()'d and not ignored:", len(conflicts)))
+		msgs = append(msgs, fmt.Sprintf("%d records that are both IGNORE*()'d and not ignored:", len(conflicts)))
 		for _, r := range conflicts {
 			msgs = append(msgs, fmt.Sprintf("    %s %s %s", r.GetLabelFQDN(), r.Type, r.GetTargetCombined()))
 		}

pkg/printer/printer.go

@@ -19,6 +19,7 @@ type CLI interface {
 	StartRegistrar(name string, skip bool)
 
 	PrintCorrection(n int, c *models.Correction)
+	PrintReport(n int, c *models.Correction) // Print corrections that are diff2.REPORT
 	EndCorrection(err error)
 	PromptToRun() bool
 }
@@ -89,6 +90,13 @@ func (c ConsolePrinter) PrintCorrection(i int, correction *models.Correction) {
 	fmt.Fprintf(c.Writer, "#%d: %s\n", i+1, correction.Msg)
 }
 
+// PrintReport is called to print/format each non-mutating correction (diff2.REPORT).
+func (c ConsolePrinter) PrintReport(i int, correction *models.Correction) {
+	// When diff1 is eliminated:
+	//fmt.Fprintf(c.Writer, "INFO#%d: %s\n", i+1, correction.Msg)
+	fmt.Fprintf(c.Writer, "INFO: %s\n", correction.Msg)
+}
+
 // PromptToRun prompts the user to see if they want to execute a correction.
 func (c ConsolePrinter) PromptToRun() bool {
 	fmt.Fprint(c.Writer, "Run? (Y/n): ")

pkg/zonerecs/zonerecords.go

@@ -7,11 +7,11 @@ import (
 // CorrectZoneRecords calls both GetZoneRecords, does any
 // post-processing, and then calls GetZoneRecordsCorrections.  The
 // name sucks because all the good names were taken.
-func CorrectZoneRecords(driver models.DNSProvider, dc *models.DomainConfig) ([]*models.Correction, error) {
+func CorrectZoneRecords(driver models.DNSProvider, dc *models.DomainConfig) ([]*models.Correction, []*models.Correction, error) {
 
 	existingRecords, err := driver.GetZoneRecords(dc.Name, dc.Metadata)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	// downcase
@@ -23,7 +23,7 @@ func CorrectZoneRecords(driver models.DNSProvider, dc *models.DomainConfig) ([]*
 	// dc.Records.
 	dc, err = dc.Copy()
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	// punycode
@@ -31,18 +31,18 @@ func CorrectZoneRecords(driver models.DNSProvider, dc *models.DomainConfig) ([]*
 	// FIXME(tlim) It is a waste to PunyCode every iteration.
 	// This should be moved to where the JavaScript is processed.
 
-	return driver.GetZoneRecordsCorrections(dc, existingRecords)
+	everything, err := driver.GetZoneRecordsCorrections(dc, existingRecords)
+	reports, corrections := splitReportsAndCorrections(everything)
+	return reports, corrections, err
 }
 
-// CountActionable returns the number of corrections that have
-// actions.  It is like `len(corrections)` but doesn't count any
-// corrections that are purely informational. (i.e. `.F` is nil)
-func CountActionable(corrections []*models.Correction) int {
-	count := 0
-	for i := range corrections {
-		if corrections[i].F != nil {
-			count++
+func splitReportsAndCorrections(everything []*models.Correction) (reports, corrections []*models.Correction) {
+	for i := range everything {
+		if everything[i].F == nil {
+			reports = append(reports, everything[i])
+		} else {
+			corrections = append(corrections, everything[i])
 		}
 	}
-	return count
+	return reports, corrections
 }

providers/route53/route53Provider.go

@@ -496,7 +496,9 @@ func (r *route53Provider) GetZoneRecordsCorrections(dc *models.DomainConfig, exi
 		return nil, err
 	}
 	instructions = reorderInstructions(instructions)
-	wasReport := false
+	var reports []*models.Correction
+
+	//wasReport := false
 	for _, inst := range instructions {
 		instNameFQDN := inst.Key.NameFQDN
 		instType := inst.Key.Type
@@ -505,8 +507,12 @@ func (r *route53Provider) GetZoneRecordsCorrections(dc *models.DomainConfig, exi
 		switch inst.Type {
 
 		case diff2.REPORT:
-			chg = r53Types.Change{}
-			wasReport = true
+			// REPORTs are held in a separate list so that they aren't part of the batching process.
+			reports = append(reports,
+				&models.Correction{
+					Msg: inst.MsgsJoined,
+				})
+			continue
 
 		case diff2.CREATE:
 			fallthrough
@@ -560,34 +566,19 @@ func (r *route53Provider) GetZoneRecordsCorrections(dc *models.DomainConfig, exi
 	}
 
 	addCorrection := func(msg string, req *r53.ChangeResourceRecordSetsInput) {
-
-		if wasReport {
-
-			// Add a "msg only" correction.
-			corrections = append(corrections,
-				&models.Correction{
-					Msg: msg,
-				})
-
-		} else {
-
-			// Add a function to execute.
-			corrections = append(corrections,
-				&models.Correction{
-					Msg: msg,
-					F: func() error {
-						var err error
-						req.HostedZoneId = zone.Id
-						withRetry(func() error {
-							_, err = r.client.ChangeResourceRecordSets(context.Background(), req)
-							return err
-						})
+		corrections = append(corrections,
+			&models.Correction{
+				Msg: msg,
+				F: func() error {
+					var err error
+					req.HostedZoneId = zone.Id
+					withRetry(func() error {
+						_, err = r.client.ChangeResourceRecordSets(context.Background(), req)
 						return err
-					},
-				})
-
-		}
-
+					})
+					return err
+				},
+			})
 	}
 
 	// Send the changes in as few API calls as possible.
@@ -605,7 +596,7 @@ func (r *route53Provider) GetZoneRecordsCorrections(dc *models.DomainConfig, exi
 		return nil, err
 	}
 
-	return corrections, nil
+	return append(reports, corrections...), nil
 
 }