From 9ddf110ad6b6b1516a502527ed21c11821c7cfeb Mon Sep 17 00:00:00 2001 From: Sam Arnold Date: Fri, 4 Feb 2022 10:59:02 -0500 Subject: [PATCH] fix: template apply uses better diff checking (#358) * fix: template apply uses better diff checking Previously, we did a DeepEqual of all the returned data about each changed entity, but due to our template overrides that is not actually all the information available for each entity. So we marked trivial things as 'conflicts' (e.g. telegraf config ID's 'changing' from the empty string to the real, current value) while not catching important conflicts like flux script changes in checks and tasks. Changes to make things more straightforward: * Change the --force behaviour to be more similar to `apt install`, where even in non-interactive mode `--force yes` is required to bypass the prompt to apply. * Before, there were two stages of diff checking - once to print diffs, and once after the 'Yes/No' prompt. If any conflicts were detected in the second check, the user got an inscrutable error message that did not highlight what the difference was or how to force it to apply. `--force conflict` was required to avoid this error. Instead, we now have simplified to `--force yes` to bypass the 'Yes/No' prompt, and we never do a second stage of diff checking. * Because we do not currently properly account for more complicated diffs (e.g. flux tasks), we now assume in the diff printing that every object has changes, except for Labels, Buckets, Variables, and Label Mappings. This could be improved in the future. * fix: when statestatus is 'remove', mark as removed --- clients/apply/apply.go | 99 +++++++------------------------ cmd/influx/apply.go | 10 +++- pkg/template/diff_printer.go | 8 ++- pkg/template/diff_printer_test.go | 24 ++++++-- 4 files changed, 56 insertions(+), 85 deletions(-) diff --git a/clients/apply/apply.go b/clients/apply/apply.go index 0ca1f91..0ac9ddb 100644 --- a/clients/apply/apply.go +++ b/clients/apply/apply.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "reflect" "strconv" "time" @@ -33,7 +32,6 @@ type Params struct { Filters []ResourceFilter Force bool - OverwriteConflicts bool Quiet bool RenderTableColors bool RenderTableBorders bool @@ -114,16 +112,12 @@ func (c Client) Apply(ctx context.Context, params *Params) error { } } - if c.StdIO.IsInteractive() && !params.Force { + if !params.Force { if confirmed := c.StdIO.GetConfirm("Confirm application of the above resources"); !confirmed { return errors.New("aborted application of template") } } - if !params.OverwriteConflicts && hasConflicts(res.Diff) { - return errors.New("template has conflicts with existing resources and cannot safely apply") - } - // Flip the dry-run flag and apply the template. req.DryRun = false res, err = c.ApplyTemplate(ctx).TemplateApply(req).Execute() @@ -167,10 +161,10 @@ func (c Client) printDiff(diff api.TemplateSummaryDiff, params *Params) error { if l.Old != nil { oldRow = buildRow(l.TemplateMetaName, hexId, *l.Old) } - if l.New != nil { + if l.New != nil && l.StateStatus != "remove" { newRow = buildRow(l.TemplateMetaName, hexId, *l.New) } - printer.AppendDiff(oldRow, newRow) + printer.AppendDiff(oldRow, newRow, false) } printer.Render() _, _ = c.StdIO.Write([]byte("\n")) @@ -199,10 +193,10 @@ func (c Client) printDiff(diff api.TemplateSummaryDiff, params *Params) error { if b.Old != nil { oldRow = buildRow(b.TemplateMetaName, hexId, *b.Old) } - if b.New != nil { + if b.New != nil && b.StateStatus != "remove" { newRow = buildRow(b.TemplateMetaName, hexId, *b.New) } - printer.AppendDiff(oldRow, newRow) + printer.AppendDiff(oldRow, newRow, false) } printer.Render() _, _ = c.StdIO.Write([]byte("\n")) @@ -223,10 +217,10 @@ func (c Client) printDiff(diff api.TemplateSummaryDiff, params *Params) error { if c.Old != nil { oldRow = buildRow(c.TemplateMetaName, hexId, *c.Old) } - if c.New != nil { + if c.New != nil && c.StateStatus != "remove" { newRow = buildRow(c.TemplateMetaName, hexId, *c.New) } - printer.AppendDiff(oldRow, newRow) + printer.AppendDiff(oldRow, newRow, true) } printer.Render() _, _ = c.StdIO.Write([]byte("\n")) @@ -247,10 +241,10 @@ func (c Client) printDiff(diff api.TemplateSummaryDiff, params *Params) error { if d.Old != nil { oldRow = buildRow(d.TemplateMetaName, hexId, *d.Old) } - if d.New != nil { + if d.New != nil && d.StateStatus != "remove" { newRow = buildRow(d.TemplateMetaName, hexId, *d.New) } - printer.AppendDiff(oldRow, newRow) + printer.AppendDiff(oldRow, newRow, true) } printer.Render() _, _ = c.StdIO.Write([]byte("\n")) @@ -267,10 +261,10 @@ func (c Client) printDiff(diff api.TemplateSummaryDiff, params *Params) error { if e.Old != nil { oldRow = buildRow(e.TemplateMetaName, hexId, *e.Old) } - if e.New != nil { + if e.New != nil && e.StateStatus != "remove" { newRow = buildRow(e.TemplateMetaName, hexId, *e.New) } - printer.AppendDiff(oldRow, newRow) + printer.AppendDiff(oldRow, newRow, true) } printer.Render() _, _ = c.StdIO.Write([]byte("\n")) @@ -292,10 +286,10 @@ func (c Client) printDiff(diff api.TemplateSummaryDiff, params *Params) error { if r.Old != nil { oldRow = buildRow(r.TemplateMetaName, hexId, *r.Old) } - if r.New != nil { + if r.New != nil && r.StateStatus != "remove" { newRow = buildRow(r.TemplateMetaName, hexId, *r.New) } - printer.AppendDiff(oldRow, newRow) + printer.AppendDiff(oldRow, newRow, true) } printer.Render() _, _ = c.StdIO.Write([]byte("\n")) @@ -316,10 +310,10 @@ func (c Client) printDiff(diff api.TemplateSummaryDiff, params *Params) error { if t.Old != nil { oldRow = buildRow(t.TemplateMetaName, hexId, *t.Old) } - if t.New != nil { + if t.New != nil && t.StateStatus != "remove" { newRow = buildRow(t.TemplateMetaName, hexId, *t.New) } - printer.AppendDiff(oldRow, newRow) + printer.AppendDiff(oldRow, newRow, true) } printer.Render() _, _ = c.StdIO.Write([]byte("\n")) @@ -351,10 +345,10 @@ func (c Client) printDiff(diff api.TemplateSummaryDiff, params *Params) error { if t.Old != nil { oldRow = buildRow(t.TemplateMetaName, hexId, *t.Old) } - if t.New != nil { + if t.New != nil && t.StateStatus != "remove" { newRow = buildRow(t.TemplateMetaName, hexId, *t.New) } - printer.AppendDiff(oldRow, newRow) + printer.AppendDiff(oldRow, newRow, true) } printer.Render() _, _ = c.StdIO.Write([]byte("\n")) @@ -378,10 +372,10 @@ func (c Client) printDiff(diff api.TemplateSummaryDiff, params *Params) error { if v.Old != nil { oldRow = buildRow(v.TemplateMetaName, hexId, *v.Old) } - if v.New != nil { + if v.New != nil && v.StateStatus != "remove" { newRow = buildRow(v.TemplateMetaName, hexId, *v.New) } - printer.AppendDiff(oldRow, newRow) + printer.AppendDiff(oldRow, newRow, false) } printer.Render() _, _ = c.StdIO.Write([]byte("\n")) @@ -398,11 +392,11 @@ func (c Client) printDiff(diff api.TemplateSummaryDiff, params *Params) error { row := []string{m.ResourceType, m.ResourceName, resId, m.LabelTemplateMetaName, m.LabelName, labelId} switch m.StateStatus { case "new": - printer.AppendDiff(nil, row) + printer.AppendDiff(nil, row, false) case "remove": - printer.AppendDiff(row, nil) + printer.AppendDiff(row, nil, false) default: - printer.AppendDiff(row, row) + printer.AppendDiff(row, row, false) } } printer.Render() @@ -411,55 +405,6 @@ func (c Client) printDiff(diff api.TemplateSummaryDiff, params *Params) error { return nil } -func hasConflicts(diff api.TemplateSummaryDiff) bool { - for _, l := range diff.Labels { - if l.StateStatus != "new" && l.Old != nil && l.New != nil && !reflect.DeepEqual(l.Old, l.New) { - return true - } - } - for _, b := range diff.Buckets { - if b.StateStatus != "new" && b.Old != nil && b.New != nil && !reflect.DeepEqual(b.Old, b.New) { - return true - } - } - for _, c := range diff.Checks { - if c.StateStatus != "new" && c.Old != nil && c.New != nil && !reflect.DeepEqual(c.Old, c.New) { - return true - } - } - for _, d := range diff.Dashboards { - if d.StateStatus != "new" && d.Old != nil && d.New != nil && !reflect.DeepEqual(d.Old, d.New) { - return true - } - } - for _, e := range diff.NotificationEndpoints { - if e.StateStatus != "new" && e.Old != nil && e.New != nil && !reflect.DeepEqual(e.Old, e.New) { - return true - } - } - for _, r := range diff.NotificationRules { - if r.StateStatus != "new" && r.Old != nil && r.New != nil && !reflect.DeepEqual(r.Old, r.New) { - return true - } - } - for _, t := range diff.TelegrafConfigs { - if t.StateStatus != "new" && t.Old != nil && t.New != nil && !reflect.DeepEqual(t.Old, t.New) { - return true - } - } - for _, t := range diff.Tasks { - if t.StateStatus != "new" && t.Old != nil && t.New != nil && !reflect.DeepEqual(t.Old, t.New) { - return true - } - } - for _, v := range diff.Variables { - if v.StateStatus != "new" && v.Old != nil && v.New != nil && !reflect.DeepEqual(v.Old, v.New) { - return true - } - } - return false -} - func (c Client) printSummary(summary api.TemplateSummaryResources, params *Params) error { if c.PrintAsJSON { return c.PrintJSON(struct { diff --git a/cmd/influx/apply.go b/cmd/influx/apply.go index c6d9bf3..603301d 100644 --- a/cmd/influx/apply.go +++ b/cmd/influx/apply.go @@ -133,7 +133,7 @@ https://github.com/influxdata/community-templates. }, &cli.StringFlag{ Name: "force", - Usage: "Set to 'true' to skip confirmation before applying changes. Set to 'conflict' to skip confirmation and overwrite existing resources", + Usage: "Set to 'yes' to skip confirmation before applying changes (recommended for non-interactive scripts).", Destination: ¶ms.force, }, &cli.StringSliceFlag{ @@ -240,12 +240,18 @@ https://github.com/influxdata/community-templates. // Parse our strange way of passing 'force' switch params.force { + case "": + // no force case "conflict": + log.Println("WARN: Passing '--force conflict' is deprecated, assuming '--force yes'") parsedParams.Force = true - parsedParams.OverwriteConflicts = true case "true": + log.Println("WARN: Passing '--force true' is deprecated, assuming '--force yes'") + parsedParams.Force = true + case "yes": parsedParams.Force = true default: + return fmt.Errorf("invalid argument '--force %s', should use '--force yes'", params.force) } api := getAPI(ctx) diff --git a/pkg/template/diff_printer.go b/pkg/template/diff_printer.go index cf7dc6c..b1a8aa3 100644 --- a/pkg/template/diff_printer.go +++ b/pkg/template/diff_printer.go @@ -106,7 +106,11 @@ func (d *DiffPrinter) Append(slc []string) { d.writer.Append(d.prepend(slc, "")) } -func (d *DiffPrinter) AppendDiff(remove, add []string) { +// AppendDiff appends a diff to the diff printer +// +// assumeDiff says to mark remove/add as a diff (with two lines), even if they are the same. +// this is used for types that the CLI does not know how to fully compare. +func (d *DiffPrinter) AppendDiff(remove, add []string, assumeDiff bool) { defer func() { d.appendCalls++ }() if d.appendCalls > 0 { @@ -127,7 +131,7 @@ func (d *DiffPrinter) AppendDiff(remove, add []string) { var ( addColors = make([]tablewriter.Colors, len(preppedAdd)) removeColors = make([]tablewriter.Colors, len(preppedRemove)) - hasDiff bool + hasDiff = assumeDiff ) addColor, removeColor := noColor, noColor if d.useColor { diff --git a/pkg/template/diff_printer_test.go b/pkg/template/diff_printer_test.go index 6fa11a9..bd533d6 100644 --- a/pkg/template/diff_printer_test.go +++ b/pkg/template/diff_printer_test.go @@ -29,19 +29,27 @@ func TestDiffPrinter(t *testing.T) { SetHeaders("Wow", "Such", "A", "Fancy", "Printer") // Add - printer.AppendDiff(nil, []string{"A", "B", "C", "D", "E"}) + printer.AppendDiff(nil, []string{"A", "B", "C", "D", "E"}, false) // No change - printer.Append([]string{"foo", "bar", "baz", "qux", "wat"}) + simple := []string{"foo", "bar", "baz", "qux", "wat"} + printer.Append(simple) + + // No change with two arguments + printer.AppendDiff(simple, simple, false) + + // No change but force a diff + printer.AppendDiff(simple, simple, true) // Replace printer.AppendDiff( []string{"1", "200000000000000", "3", "4", "5"}, []string{"9", "8", "7", "6", "5"}, + false, ) // Remove - printer.AppendDiff([]string{"x y", "z x", "x y z", "", "y z"}, nil) + printer.AppendDiff([]string{"x y", "z x", "x y z", "", "y z"}, nil, false) printer.Render() expected := `EXAMPLE +add | -remove | unchanged @@ -53,6 +61,14 @@ func TestDiffPrinter(t *testing.T) { | | foo | bar | baz | qux | wat | +-----+-----+-----------------+-------+-------+---------+ +-----+-----+-----------------+-------+-------+---------+ +| | foo | bar | baz | qux | wat | ++-----+-----+-----------------+-------+-------+---------+ ++-----+-----+-----------------+-------+-------+---------+ +| - | foo | bar | baz | qux | wat | ++-----+-----+-----------------+-------+-------+---------+ +| + | foo | bar | baz | qux | wat | ++-----+-----+-----------------+-------+-------+---------+ ++-----+-----+-----------------+-------+-------+---------+ | - | 1 | 200000000000000 | 3 | 4 | 5 | +-----+-----+-----------------+-------+-------+---------+ | + | 9 | 8 | 7 | 6 | 5 | @@ -60,7 +76,7 @@ func TestDiffPrinter(t *testing.T) { +-----+-----+-----------------+-------+-------+---------+ | - | x y | z x | x y z | | y z | +-----+-----+-----------------+-------+-------+---------+ -| TOTAL | 3 | +| TOTAL | 5 | +-----+-----+-----------------+-------+-------+---------+ ` require.Equal(t, expected, out.String())