CHORE: Add deprecation messages to preview/push/ppreview/ppush commands (#3155)

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

commands/ppreviewPush.go

@@ -28,12 +28,46 @@ type zoneCache struct {
 	sync.Mutex
 }
 
+const cmodewarn = `WARN: In v4.15 --cmode will default to "concurrent".` +
+	` Please test and report any bugs ASAP.` +
+	` In v4.16 or later, "legacy" will go away.` +
+	` See https://docs.dnscontrol.org/commands/preview-push` +
+	"\n"
+
+const ppreviewwarn = `WARN: ppreview is going away in v4.16 or later.` +
+	` Use "preview --cmode=concurrent" instead.` +
+	"\n"
+
+const ppushwarn = `WARN: ppush is going away in v4.16 or later.` +
+	` Use "push --cmode=concurrent" 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, cmodewarn)
+				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: "read live configuration and identify changes to be made, without applying them",
+		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(),
@@ -79,11 +113,11 @@ func (args *PPreviewArgs) flags() []cli.Flag {
 	flags = append(flags, &cli.StringFlag{
 		Name:        "cmode",
 		Destination: &args.ConcurMode,
-		Value:       "default",
-		Usage:       `Which providers to run concurrently: all, default, none`,
+		Value:       "legacy",
+		Usage:       `Which providers to run concurrently: legacy, concurrent, none, all`,
 		Action: func(c *cli.Context, s string) error {
-			if !slices.Contains([]string{"all", "default", "none"}, s) {
-				fmt.Printf("%q is not a valid option for --cmode.  Valie are: all, default, none", s)
+			if !slices.Contains([]string{"legacy", "concurrent", "none", "all"}, s) {
+				fmt.Printf("%q is not a valid option for --cmode.  Values are: legacy, concurrent, none, all\n", s)
 			}
 			return nil
 		},
@@ -125,12 +159,32 @@ func (args *PPreviewArgs) flags() []cli.Flag {
 	return flags
 }
 
+var _ = cmd(catMain, func() *cli.Command {
+	var args PPushArgs
+	return &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, cmodewarn)
+				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(),
@@ -160,7 +214,7 @@ func PPreview(args PPreviewArgs) error {
 
 // PPush implements the push subcommand.
 func PPush(args PPushArgs) error {
-	return prun(args.PPreviewArgs, true, args.Interactive, printer.DefaultPrinter, args.Report)
+	return run(args.PPreviewArgs, true, args.Interactive, printer.DefaultPrinter, &args.Report)
 }
 
 var pobsoleteDiff2FlagUsed = false
@@ -363,9 +417,7 @@ func optimizeOrder(zones []*models.DomainConfig) []*models.DomainConfig {
 
 func oneZone(zone *models.DomainConfig, args PPreviewArgs, zc *zoneCache) {
 	// Fix the parent zone's delegation: (if able/needed)
-	//zone.NameserversMutex.Lock()
 	delegationCorrections, dcCount := generateDelegationCorrections(zone, zone.DNSProviderInstances, zone.RegistrarInstance)
-	//zone.NameserversMutex.Unlock()
 
 	// Loop over the (selected) providers configured for that zone:
 	providersToProcess := whichProvidersToProcess(zone.DNSProviderInstances, args.Providers)

commands/previewPush.go

@@ -10,7 +10,6 @@ import (
 	"golang.org/x/net/idna"
 
 	"github.com/StackExchange/dnscontrol/v4/models"
-	"github.com/StackExchange/dnscontrol/v4/pkg/bindserial"
 	"github.com/StackExchange/dnscontrol/v4/pkg/credsfile"
 	"github.com/StackExchange/dnscontrol/v4/pkg/nameservers"
 	"github.com/StackExchange/dnscontrol/v4/pkg/normalize"
@@ -19,34 +18,9 @@ import (
 	"github.com/StackExchange/dnscontrol/v4/pkg/rfc4183"
 	"github.com/StackExchange/dnscontrol/v4/pkg/zonerecs"
 	"github.com/StackExchange/dnscontrol/v4/providers"
-	"github.com/urfave/cli/v2"
 	"golang.org/x/exp/slices"
 )
 
-var _ = cmd(catMain, func() *cli.Command {
-	var args PreviewArgs
-	return &cli.Command{
-		Name:  "preview",
-		Usage: "read live configuration and identify changes to be made, without applying them",
-		Action: func(ctx *cli.Context) error {
-			return exit(Preview(args))
-		},
-		Flags: args.flags(),
-	}
-}())
-
-// PreviewArgs contains all data/flags needed to run preview, independently of CLI
-type PreviewArgs struct {
-	GetDNSConfigArgs
-	GetCredentialsArgs
-	FilterArgs
-	Notify      bool
-	WarnChanges bool
-	NoPopulate  bool
-	Full        bool
-	Report      string
-}
-
 // ReportItem is a record of corrections for a particular domain/provider/registrar.
 type ReportItem struct {
 	Domain      string `json:"domain"`
@@ -55,94 +29,20 @@ type ReportItem struct {
 	Registrar   string `json:"registrar,omitempty"`
 }
 
-func (args *PreviewArgs) flags() []cli.Flag {
-	flags := args.GetDNSConfigArgs.flags()
-	flags = append(flags, args.GetCredentialsArgs.flags()...)
-	flags = append(flags, args.FilterArgs.flags()...)
-	flags = append(flags, &cli.BoolFlag{
-		Name:        "notify",
-		Destination: &args.Notify,
-		Usage:       `set to true to send notifications to configured destinations`,
-	})
-	flags = append(flags, &cli.BoolFlag{
-		Name:        "expect-no-changes",
-		Destination: &args.WarnChanges,
-		Usage:       `set to true for non-zero return code if there are changes`,
-	})
-	flags = append(flags, &cli.BoolFlag{
-		Name:        "no-populate",
-		Destination: &args.NoPopulate,
-		Usage:       `Use this flag to not auto-create non-existing zones at the provider`,
-	})
-	flags = append(flags, &cli.BoolFlag{
-		Name:        "full",
-		Destination: &args.Full,
-		Usage:       `Add headings, providers names, notifications of no changes, etc`,
-	})
-	flags = append(flags, &cli.IntFlag{
-		Name:   "reportmax",
-		Hidden: true,
-		Usage:  `Limit the IGNORE/NO_PURGE report to this many lines (Expermental. Will change in the future.)`,
-		Action: func(ctx *cli.Context, max int) error {
-			printer.MaxReport = max
-			return nil
-		},
-	})
-	flags = append(flags, &cli.Int64Flag{
-		Name:        "bindserial",
-		Destination: &bindserial.ForcedValue,
-		Usage:       `Force BIND serial numbers to this value (for reproducibility)`,
-	})
-	flags = append(flags, &cli.StringFlag{
-		Name:        "report",
-		Destination: &args.Report,
-		Usage:       `Generate a machine-parseable report of corrections counts.`,
-	})
-	return flags
-}
-
-var _ = cmd(catMain, func() *cli.Command {
-	var args PushArgs
-	return &cli.Command{
-		Name:  "push",
-		Usage: "identify changes to be made, and perform them",
-		Action: func(ctx *cli.Context) error {
-			return exit(Push(args))
-		},
-		Flags: args.flags(),
-	}
-}())
-
-// PushArgs contains all data/flags needed to run push, independently of CLI
-type PushArgs struct {
-	PreviewArgs
-	Interactive bool
-}
-
-func (args *PushArgs) flags() []cli.Flag {
-	flags := args.PreviewArgs.flags()
-	flags = append(flags, &cli.BoolFlag{
-		Name:        "i",
-		Destination: &args.Interactive,
-		Usage:       "Interactive. Confirm or Exclude each correction before they run",
-	})
-	return flags
-}
-
 // Preview implements the preview subcommand.
-func Preview(args PreviewArgs) error {
+func Preview(args PPreviewArgs) error {
 	return run(args, false, false, printer.DefaultPrinter, &args.Report)
 }
 
 // Push implements the push subcommand.
-func Push(args PushArgs) error {
-	return run(args.PreviewArgs, true, args.Interactive, printer.DefaultPrinter, &args.Report)
+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 PreviewArgs, push bool, interactive bool, out printer.CLI, report *string) error {
+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.

documentation/preview-push.md

@@ -91,23 +91,39 @@ OPTIONS:
     serial number generator to output the value specified for all domains. This is
     generally used for reproducibility in testing pipelines.
 
+* `--cmode value`
+  * Concurrency mode. See below.
+
 * `--report name`
   * Write a machine-parseable report of
     corrections to the file named `name`. If no name is specified, no
     report is generated. See [JSON Reports](json-reports.md)
 
+## cmode
+
+The `preview`/`push` commands begin with a data-gathering phase that collects current configuration
+from providers and zones.  This collection can be done sequentially or concurrently.  Concurrently is significantly faster.  However since concurrent mode is newer, not all providers have been tested and certified as being compatible with this mode.  Therefore the `--cmode` flag can be used to control concurrency.
+
+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.
+* `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.
+* `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.
+
 ## ppreview/ppush
 
-{% hint style="info" %}
-Starting in v4.9
+{% hint style="warning" %}
+These commands will go away in v4.16 or later.  Starting in v4.14, please use
+`preview`/`push` with `--cmode concurrent` instead.
 {% 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.
-
-* `--cmode value`
-  * Concurrency mode. Specifies what kind of providers should be run concurrently.
-    * `default` -- Providers are run 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.
-    * `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.
+they replace the existing `preview`/`push` commands.