FEATURE: Remove ppreview/ppush and cmode=legacy (#3412)

Co-authored-by: Jeffrey Cafferata <jeffrey@jcid.nl>

commands/commands.go

@@ -64,7 +64,7 @@ func Run(v string) int {
 			Usage:  "Obsolete flag. Will be removed in v5 or later",
 			Hidden: true,
 			Action: func(ctx *cli.Context, v bool) error {
-				obsoleteDiff2FlagUsed = true
+				pobsoleteDiff2FlagUsed = true
 				return nil
 			},
 		},
@@ -304,33 +304,6 @@ func (args *FilterArgs) flags() []cli.Flag {
 	}
 }
 
-func (args *FilterArgs) shouldRunProvider(name string, dc *models.DomainConfig) bool {
-	if args.Providers == "all" {
-		return true
-	}
-	if args.Providers == "" {
-		for _, pri := range dc.DNSProviderInstances {
-			if pri.Name == name {
-				return pri.IsDefault
-			}
-		}
-		return true
-	}
-	for _, prov := range strings.Split(args.Providers, ",") {
-		if prov == name {
-			return true
-		}
-	}
-	return false
-}
-
-func (args *FilterArgs) shouldRunDomain(d string) bool {
-	if args.Domains == "" {
-		return true
-	}
-	return domainInList(d, strings.Split(args.Domains, ","))
-}
-
 func domainInList(domain string, list []string) bool {
 	for _, item := range list {
 		if strings.HasPrefix(item, "*") && strings.HasSuffix(domain, item[1:]) {

commands/ppreviewPush.go

@@ -29,45 +29,12 @@ type zoneCache struct {
 	sync.Mutex
 }
 
-const legacywarn = `WARNING: --cmode=legacy will go away in v4.16 or later.` +
-	` Please test --cmode=concurrent and report any bugs ASAP.` +
-	` See https://docs.dnscontrol.org/commands/preview-push#cmode` +
-	"\n"
-
-const ppreviewwarn = `WARNING: ppreview is going away in v4.16 or later.` +
-	` Use "preview" instead.` +
-	"\n"
-
-const ppushwarn = `WARNING: ppush is going away in v4.16 or later.` +
-	` Use "push" instead.` +
-	"\n"
-
 var _ = cmd(catMain, func() *cli.Command {
 	var args PPreviewArgs
 	return &cli.Command{
 		Name:  "preview",
 		Usage: "read live configuration and identify changes to be made, without applying them",
 		Action: func(ctx *cli.Context) error {
-			if args.ConcurMode == "legacy" {
-				fmt.Fprint(os.Stderr, legacywarn)
-				return exit(Preview(args))
-			}
-			return exit(PPreview(args))
-		},
-		Flags: args.flags(),
-	}
-}())
-
-var _ = cmd(catMain, func() *cli.Command {
-	var args PPreviewArgs
-	return &cli.Command{
-		Name:  "ppreview",
-		Usage: "Deprecated. Same as: preview --cmode=concurrent",
-		Action: func(ctx *cli.Context) error {
-			fmt.Fprint(os.Stderr, ppreviewwarn)
-			if args.ConcurMode == "legacy" {
-				return exit(Preview(args))
-			}
 			return exit(PPreview(args))
 		},
 		Flags: args.flags(),
@@ -115,10 +82,11 @@ func (args *PPreviewArgs) flags() []cli.Flag {
 		Name:        "cmode",
 		Destination: &args.ConcurMode,
 		Value:       "concurrent",
-		Usage:       `Which providers to run concurrently: legacy, concurrent, none, all`,
+		Usage:       `Which providers to run concurrently: concurrent, none, all`,
 		Action: func(c *cli.Context, s string) error {
-			if !slices.Contains([]string{"legacy", "concurrent", "none", "all"}, s) {
+			if !slices.Contains([]string{"concurrent", "none", "all"}, s) {
-				fmt.Printf("%q is not a valid option for --cmode.  Values are: legacy, concurrent, none, all\n", s)
+				fmt.Printf("%q is not a valid option for --cmode.  Values are: concurrent, none, all\n", s)
+				os.Exit(1)
 			}
 			return nil
 		},
@@ -172,26 +140,6 @@ var _ = cmd(catMain, func() *cli.Command {
 		Name:  "push",
 		Usage: "identify changes to be made, and perform them",
 		Action: func(ctx *cli.Context) error {
-			if args.ConcurMode == "legacy" {
-				fmt.Fprint(os.Stderr, legacywarn)
-				return exit(Push(args))
-			}
-			return exit(PPush(args))
-		},
-		Flags: args.flags(),
-	}
-}())
-
-var _ = cmd(catMain, func() *cli.Command {
-	var args PPushArgs
-	return &cli.Command{
-		Name:  "ppush",
-		Usage: "identify changes to be made, and perform them",
-		Action: func(ctx *cli.Context) error {
-			fmt.Fprint(os.Stderr, ppushwarn)
-			if args.ConcurMode == "legacy" {
-				return exit(Push(args))
-			}
 			return exit(PPush(args))
 		},
 		Flags: args.flags(),
@@ -280,13 +228,13 @@ func prun(args PPreviewArgs, push bool, interactive bool, out printer.CLI, repor
 			out.PrintfIf(fullMode, "Concurrently checking for zone: %q\n", zone.Name)
 			go func(zone *models.DomainConfig) {
 				defer wg.Done()
-				oneZonePopulate(zone, args, zcache)
+				oneZonePopulate(zone, zcache)
 			}(zone)
 		}
 		out.PrintfIf(fullMode, "SERIALLY checking for %d zone(s)\n", len(zonesSerial))
 		for _, zone := range zonesSerial {
 			out.PrintfIf(fullMode, "Serially checking for zone: %q\n", zone.Name)
-			oneZonePopulate(zone, args, zcache)
+			oneZonePopulate(zone, zcache)
 		}
 		out.PrintfIf(fullMode && len(zonesConcurrent) > 0, "Waiting for concurrent checking(s) to complete...")
 		wg.Wait()
@@ -468,7 +416,7 @@ func optimizeOrder(zones []*models.DomainConfig) []*models.DomainConfig {
 	return zones
 }
 
-func oneZonePopulate(zone *models.DomainConfig, args PPreviewArgs, zc *zoneCache) {
+func oneZonePopulate(zone *models.DomainConfig, zc *zoneCache) {
 	// Loop over all the providers configured for that zone:
 	for _, provider := range zone.DNSProviderInstances {
 		populateCorrections := generatePopulateCorrections(provider, zone.Name, zc)

commands/previewPush.go

@@ -1,24 +1,13 @@
 package commands
 
 import (
-	"encoding/json"
-	"errors"
 	"fmt"
 	"os"
 	"strings"
-	"sync"
 
 	"github.com/StackExchange/dnscontrol/v4/models"
-	"github.com/StackExchange/dnscontrol/v4/pkg/credsfile"
-	"github.com/StackExchange/dnscontrol/v4/pkg/nameservers"
-	"github.com/StackExchange/dnscontrol/v4/pkg/normalize"
 	"github.com/StackExchange/dnscontrol/v4/pkg/notifications"
-	"github.com/StackExchange/dnscontrol/v4/pkg/printer"
-	"github.com/StackExchange/dnscontrol/v4/pkg/rfc4183"
-	"github.com/StackExchange/dnscontrol/v4/pkg/zonerecs"
 	"github.com/StackExchange/dnscontrol/v4/providers"
-	"golang.org/x/exp/slices"
-	"golang.org/x/net/idna"
 )
 
 // ReportItem is a record of corrections for a particular domain/provider/registrar.
@@ -29,197 +18,6 @@ type ReportItem struct {
 	Registrar   string `json:"registrar,omitempty"`
 }
 
-// Preview implements the preview subcommand.
-func Preview(args PPreviewArgs) error {
-	return run(args, false, false, printer.DefaultPrinter, &args.Report)
-}
-
-// Push implements the push subcommand.
-func Push(args PPushArgs) error {
-	return run(args.PPreviewArgs, true, args.Interactive, printer.DefaultPrinter, &args.Report)
-}
-
-var obsoleteDiff2FlagUsed = false
-
-// run is the main routine common to preview/push
-func run(args PPreviewArgs, push bool, interactive bool, out printer.CLI, report *string) error {
-	// TODO: make truly CLI independent. Perhaps return results on a channel as they occur
-
-	// This is a hack until we have the new printer replacement.
-	printer.SkinnyReport = !args.Full
-
-	if obsoleteDiff2FlagUsed {
-		printer.Println("WARNING: Please remove obsolete --diff2 flag. This will be an error in v5 or later. See https://github.com/StackExchange/dnscontrol/issues/2262")
-	}
-
-	cfg, err := GetDNSConfig(args.GetDNSConfigArgs)
-	if err != nil {
-		return err
-	}
-	providerConfigs, err := credsfile.LoadProviderConfigs(args.CredsFile)
-	if err != nil {
-		return err
-	}
-	notifier, err := InitializeProviders(cfg, providerConfigs, args.Notify)
-	if err != nil {
-		return err
-	}
-
-	errs := normalize.ValidateAndNormalizeConfig(cfg)
-	if PrintValidationErrors(errs) {
-		return errors.New("exiting due to validation errors")
-	}
-	anyErrors := false
-	totalCorrections := 0
-
-	// create a WaitGroup with the length of domains for the anonymous functions (later goroutines) to wait for
-	var wg sync.WaitGroup
-	wg.Add(len(cfg.Domains))
-	var reportItems []ReportItem
-	// For each domain in dnsconfig.js...
-	for _, domain := range cfg.Domains {
-		// Run preview or push operations per domain as anonymous function, in preparation for the later use of goroutines.
-		// For now running this code is still sequential.
-		// Please note that at the end of this anonymous function there is a } (domain) which executes this function actually
-		func(domain *models.DomainConfig) {
-			defer wg.Done() // defer notify WaitGroup this anonymous function has finished
-
-			uniquename := domain.GetUniqueName()
-			if !args.shouldRunDomain(uniquename) {
-				return
-			}
-
-			err = domain.Punycode()
-			if err != nil {
-				return
-			}
-
-			// Correct the domain...
-
-			out.StartDomain(uniquename)
-			var providersWithExistingZone []*models.DNSProviderInstance
-			/// For each DSP...
-			for _, provider := range domain.DNSProviderInstances {
-				if !args.NoPopulate {
-					// preview run: check if zone is already there, if not print a warning
-					if lister, ok := provider.Driver.(providers.ZoneLister); ok && !push {
-						zones, err := lister.ListZones()
-						if err != nil {
-							out.Errorf("ERROR: %s\n", err.Error())
-							anyErrors = true
-							return
-						}
-						aceZoneName, _ := idna.ToASCII(domain.Name)
-
-						if !slices.Contains(zones, aceZoneName) {
-							// out.Warnf("DEBUG: zones: %v\n", zones)
-							// out.Warnf("DEBUG: Name: %v\n", domain.Name)
-
-							out.Warnf("Zone '%s' does not exist in the '%s' profile and will be added automatically.\n", domain.Name, provider.Name)
-							continue // continue with next provider, as we can not determine corrections without an existing zone
-						}
-					} else if creator, ok := provider.Driver.(providers.ZoneCreator); ok && push {
-						// this is the actual push, ensure domain exists at DSP
-						if err := creator.EnsureZoneExists(domain.Name); err != nil {
-							out.Warnf("Error creating domain: %s\n", err)
-							anyErrors = true
-							continue // continue with next provider, as we couldn't create this one
-						}
-					}
-				}
-				providersWithExistingZone = append(providersWithExistingZone, provider)
-			}
-
-			// Correct the registrar...
-
-			nsList, err := nameservers.DetermineNameserversForProviders(domain, providersWithExistingZone, false)
-			if err != nil {
-				out.Errorf("ERROR: %s\n", err.Error())
-				return
-			}
-			domain.Nameservers = nsList
-			nameservers.AddNSRecords(domain)
-
-			for _, provider := range providersWithExistingZone {
-				shouldrun := args.shouldRunProvider(provider.Name, domain)
-				out.StartDNSProvider(provider.Name, !shouldrun)
-				if !shouldrun {
-					continue
-				}
-
-				reports, corrections, actualChangeCount, err := zonerecs.CorrectZoneRecords(provider.Driver, domain)
-				out.EndProvider(provider.Name, actualChangeCount, err)
-				if err != nil {
-					anyErrors = true
-					return
-				}
-				totalCorrections += actualChangeCount
-				printReports(domain.Name, provider.Name, reports, out, push, notifier)
-				reportItems = append(reportItems, ReportItem{
-					Domain:      domain.Name,
-					Corrections: actualChangeCount,
-					Provider:    provider.Name,
-				})
-				anyErrors = printOrRunCorrections(domain.Name, provider.Name, corrections, out, push, interactive, notifier) || anyErrors
-			}
-
-			//
-			run := args.shouldRunProvider(domain.RegistrarName, domain)
-			out.StartRegistrar(domain.RegistrarName, !run)
-			if !run {
-				return
-			}
-			if len(domain.Nameservers) == 0 && domain.Metadata["no_ns"] != "true" {
-				out.Warnf("No nameservers declared; skipping registrar. Add {no_ns:'true'} to force.\n")
-				return
-			}
-
-			corrections, err := domain.RegistrarInstance.Driver.GetRegistrarCorrections(domain)
-			out.EndProvider(domain.RegistrarName, len(corrections), err)
-			if err != nil {
-				anyErrors = true
-				return
-			}
-			totalCorrections += len(corrections)
-			reportItems = append(reportItems, ReportItem{
-				Domain:      domain.Name,
-				Corrections: len(corrections),
-				Registrar:   domain.RegistrarName,
-			})
-			anyErrors = printOrRunCorrections(domain.Name, domain.RegistrarName, corrections, out, push, interactive, notifier) || anyErrors
-		}(domain)
-	}
-	wg.Wait() // wait for all anonymous functions to finish
-
-	if os.Getenv("TEAMCITY_VERSION") != "" {
-		fmt.Fprintf(os.Stderr, "##teamcity[buildStatus status='SUCCESS' text='%d corrections']", totalCorrections)
-	}
-	rfc4183.PrintWarning()
-	notifier.Done()
-	out.Printf("Done. %d corrections.\n", totalCorrections)
-	if anyErrors {
-		return errors.New("completed with errors")
-	}
-	if totalCorrections != 0 && args.WarnChanges {
-		return errors.New("there are pending changes")
-	}
-	if report != nil && *report != "" {
-		f, err := os.OpenFile(*report, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o644)
-		if err != nil {
-			return err
-		}
-		defer f.Close()
-		b, err := json.MarshalIndent(reportItems, "", "  ")
-		if err != nil {
-			return err
-		}
-		if _, err := f.Write(b); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
 // InitializeProviders takes (fully processed) configuration and instantiates all providers and returns them.
 func InitializeProviders(cfg *models.DNSConfig, providerConfigs map[string]map[string]string, notifyFlag bool) (notify notifications.Notifier, err error) {
 	var notificationCfg map[string]string
@@ -467,40 +265,3 @@ func refineProviderType(credEntryName string, t string, credFields map[string]st
 		return ct, "", nil
 	}
 }
-
-func printOrRunCorrections(domain string, provider string, corrections []*models.Correction, out printer.CLI, push bool, interactive bool, notifier notifications.Notifier) (anyErrors bool) {
-	anyErrors = false
-	if len(corrections) == 0 {
-		return false
-	}
-	for i, correction := range corrections {
-		out.PrintCorrection(i, correction)
-		var err error
-		if push {
-			if interactive && !out.PromptToRun() {
-				continue
-			}
-			if correction.F != nil {
-				err = correction.F()
-				out.EndCorrection(err)
-				if err != nil {
-					anyErrors = true
-				}
-			}
-		}
-		notifier.Notify(domain, provider, correction.Msg, err, !push)
-	}
-	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
-}

documentation/preview-push.md

@@ -106,24 +106,20 @@ from providers and zones.  This collection can be done sequentially or concurren
 
 The `--cmode` value may be one of the following:
 
-* `legacy` -- Use the older, sequential code.  All data is gathered sequentially. This option and the related code will removed in release v4.16 (or later).  Please test `--cmode concurrent` and [report any bugs](https://github.com/StackExchange/dnscontrol/issues) ASAP.
+* `legacy` -- Use the older, sequential code.  All data is gathered sequentially. This option is removed as of release v4.16.
 * `concurrent` -- Gathering is done either sequentially or concurrently depending on whether the provider is marked as having been tested to run concurrently.
-* `none` -- All providers are run sequentially. This is the safest mode. It can be used if a concurrency bug is discovered.  While this is logically the same as `legacy`, it is implemented using the newer concurrent code, with concurrency disabled.
+* `none` -- All providers are run sequentially. This is the safest mode. It can be used if a concurrency bug is discovered.
 * `all` -- This is unsafe. It runs all providers concurrently, even the ones that have not be validated to run concurrently. It is generally only used for demonstrating bugs.
 
 The default value of `--cmode` will change over time:
 
 * v4.14: `--cmode legacy`
 * v4.15: `--cmode concurrent`
-* v4.16 or later (target 1-Jan-2025): The `--cmode legacy` option will be removed, along with the old serial code.
+* v4.16: The `--cmode legacy` option was removed, along with the old serial code.
 
 ## ppreview/ppush
 
 {% hint style="warning" %}
-These commands will go away in v4.16 or later.  Starting in v4.14, please use
+`ppreview`/`ppush` are the same as `preview`/`push` with `--cmode concurrent` instead.
-`preview`/`push` with `--cmode concurrent` instead.
+These commands were for testing and were removed in v4.16.
 {% endhint %}
-
-The `ppreview`/`ppush` subcommands are a preview of a future feature where zone
-data is gathered concurrently. The commands will go away when
-they replace the existing `preview`/`push` commands.