Skip to content

Commit

Permalink
Refactor global varibales for diff options
Browse files Browse the repository at this point in the history
The usage of global variables makes it harder to test the package and is
generally not the best pattern for non constant settings.

Introduce compare options that can optionally be used during the compare
to specify the way the comparison is done, for example whether to ignore
order changes in lists.

Move style settings for Go Patch style and minor change threshold into
the report source file.

Move code that is only used for the human report into the respective
source code file.

Introduce test case to show that the identification of non-standard
named entry list identifiers can fail if configured differently.

Refactor code to match a more common stlye when it comes to multiline
instructions with lots of parameters.
  • Loading branch information
HeavyWombat committed Aug 20, 2020
1 parent 16e3465 commit b2a2e36
Show file tree
Hide file tree
Showing 6 changed files with 213 additions and 139 deletions.
24 changes: 14 additions & 10 deletions internal/cmd/between.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ type betweenCmdOptions struct {
exitWithCount bool
translateListToDocuments bool
omitHeader bool
useGoPatchPaths bool
ignoreOrderChanges bool
chroot string
chrootFrom string
chrootTo string
Expand Down Expand Up @@ -82,19 +84,19 @@ types are: YAML (http://yaml.org/) and JSON (http://json.org/).

// Change root of 'from' input file if change root flag for 'from' is set
if betweenCmdSettings.chrootFrom != "" {
if err = dyff.ChangeRoot(&from, betweenCmdSettings.chrootFrom, betweenCmdSettings.translateListToDocuments); err != nil {
if err = dyff.ChangeRoot(&from, betweenCmdSettings.chrootFrom, betweenCmdSettings.useGoPatchPaths, betweenCmdSettings.translateListToDocuments); err != nil {
return wrap.Errorf(err, "failed to change root of %s to path %s", from.Location, betweenCmdSettings.chrootFrom)
}
}

// Change root of 'to' input file if change root flag for 'to' is set
if betweenCmdSettings.chrootTo != "" {
if err = dyff.ChangeRoot(&to, betweenCmdSettings.chrootTo, betweenCmdSettings.translateListToDocuments); err != nil {
if err = dyff.ChangeRoot(&to, betweenCmdSettings.chrootTo, betweenCmdSettings.useGoPatchPaths, betweenCmdSettings.translateListToDocuments); err != nil {
return wrap.Errorf(err, "failed to change root of %s to path %s", to.Location, betweenCmdSettings.chrootTo)
}
}

report, err := dyff.CompareInputFiles(from, to)
report, err := dyff.CompareInputFiles(from, to, dyff.IgnoreOrderChanges(betweenCmdSettings.ignoreOrderChanges))
if err != nil {
return wrap.Errorf(err, "failed to compare input files")
}
Expand All @@ -103,10 +105,12 @@ types are: YAML (http://yaml.org/) and JSON (http://json.org/).
switch strings.ToLower(betweenCmdSettings.style) {
case "human", "bosh":
reportWriter = &dyff.HumanReport{
Report: report,
DoNotInspectCerts: betweenCmdSettings.doNotInspectCerts,
NoTableStyle: betweenCmdSettings.noTableStyle,
ShowBanner: !betweenCmdSettings.omitHeader,
Report: report,
DoNotInspectCerts: betweenCmdSettings.doNotInspectCerts,
NoTableStyle: betweenCmdSettings.noTableStyle,
ShowBanner: !betweenCmdSettings.omitHeader,
UseGoPatchPaths: betweenCmdSettings.useGoPatchPaths,
MinorChangeThreshold: 0.1,
}

case "brief", "short", "summary":
Expand Down Expand Up @@ -151,10 +155,10 @@ func init() {
// Human/BOSH output related flags
betweenCmd.PersistentFlags().BoolVarP(&betweenCmdSettings.noTableStyle, "no-table-style", "l", false, "do not place blocks next to each other, always use one row per text block")
betweenCmd.PersistentFlags().BoolVarP(&betweenCmdSettings.doNotInspectCerts, "no-cert-inspection", "x", false, "disable x509 certificate inspection, compare as raw text")
betweenCmd.PersistentFlags().BoolVarP(&betweenCmdSettings.useGoPatchPaths, "use-go-patch-style", "g", false, "use Go-Patch style paths in outputs")

// General `dyff` package related preferences
betweenCmd.PersistentFlags().BoolVarP(&dyff.UseGoPatchPaths, "use-go-patch-style", "g", false, "use Go-Patch style paths in outputs")
betweenCmd.PersistentFlags().BoolVarP(&dyff.IgnoreOrderChanges, "ignore-order-changes", "i", false, "ignore order changes in lists")
// Compare options
betweenCmd.PersistentFlags().BoolVarP(&betweenCmdSettings.ignoreOrderChanges, "ignore-order-changes", "i", false, "ignore order changes in lists")

// Input documents modification flags
betweenCmd.PersistentFlags().BoolVar(&betweenCmdSettings.swap, "swap", false, "Swap 'from' and 'to' for comparison")
Expand Down
102 changes: 75 additions & 27 deletions pkg/dyff/compare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,18 @@ listY: [ Yo, Yo, Yo ]
}))
})

It("should not return order changes in named entry lists in case the ignore option is enabled", func() {
results, err := compare(
yml(`list: [ {name: A}, {name: C}, {name: B}, {name: D}, {name: E} ]`),
yml(`list: [ {name: A}, {name: B}, {name: C}, {name: D}, {name: E} ]`),
IgnoreOrderChanges(true),
)

Expect(err).To(BeNil())
Expect(results).NotTo(BeNil())
Expect(len(results)).To(BeEquivalentTo(0))
})

It("should return order changes in simple lists (ignoring additions and removals)", func() {
from := yml(`list: [ A, C, B, D, E ]`)
to := yml(`list: [ A, X1, B, C, D, X2 ]`)
Expand Down Expand Up @@ -644,39 +656,75 @@ listY: [ Yo, Yo, Yo ]
})

It("should return differences in named lists even if no standard identifier is used", func() {
results, err := CompareInputFiles(file("../../assets/prometheus/from.yml"), file("../../assets/prometheus/to.yml"))
expected := []Diff{
singleDiff("/scrape_configs", ORDERCHANGE, []string{
"kubernetes-nodes",
"kubernetes-apiservers",
"kubernetes-cadvisor",
"kubernetes-service-endpoints",
"kubernetes-services",
"kubernetes-ingresses",
"kubernetes-pods",
}, []string{
"kubernetes-apiservers",
"kubernetes-nodes",
"kubernetes-cadvisor",
"kubernetes-service-endpoints",
"kubernetes-services",
"kubernetes-ingresses",
"kubernetes-pods",
}),

singleDiff("/scrape_configs/job_name=kubernetes-apiservers/scheme", MODIFICATION, "http", "https"),

singleDiff("/scrape_configs/job_name=kubernetes-apiservers/relabel_configs/0/regex", MODIFICATION, "default;kubernetes;http", "default;kubernetes;https"),
}
results, err := CompareInputFiles(
file("../../assets/prometheus/from.yml"),
file("../../assets/prometheus/to.yml"),
)

Expect(err).To(BeNil())
Expect(results).NotTo(BeNil())
Expect(results.Diffs).NotTo(BeNil())

expected := []Diff{
singleDiff("/scrape_configs", ORDERCHANGE,
[]string{
"kubernetes-nodes",
"kubernetes-apiservers",
"kubernetes-cadvisor",
"kubernetes-service-endpoints",
"kubernetes-services",
"kubernetes-ingresses",
"kubernetes-pods",
},
[]string{
"kubernetes-apiservers",
"kubernetes-nodes",
"kubernetes-cadvisor",
"kubernetes-service-endpoints",
"kubernetes-services",
"kubernetes-ingresses",
"kubernetes-pods",
},
),

singleDiff("/scrape_configs/job_name=kubernetes-apiservers/scheme", MODIFICATION,
"http",
"https",
),

singleDiff("/scrape_configs/job_name=kubernetes-apiservers/relabel_configs/0/regex", MODIFICATION,
"default;kubernetes;http",
"default;kubernetes;https",
),
}

Expect(len(results.Diffs)).To(BeEquivalentTo(len(expected)))
for i, diff := range results.Diffs {
Expect(diff).To(BeSameDiffAs(expected[i]))
}
})

for i, result := range results.Diffs {
Expect(result).To(BeSameDiffAs(expected[i]))
It("should fail to find the non-standard identifier if the threshold is too high", func() {
report, err := CompareInputFiles(
file("../../assets/prometheus/from.yml"),
file("../../assets/prometheus/to.yml"),
NonStandardIdentifierGuessCountThreshold(8),
)

Expect(err).To(BeNil())
Expect(report).NotTo(BeNil())
Expect(report.Diffs).NotTo(BeNil())

var orderChangeDiffs = 0
for _, diff := range report.Diffs {
for _, detail := range diff.Details {
if detail.Kind == ORDERCHANGE {
orderChangeDiffs++
}
}
}

Expect(orderChangeDiffs).To(BeEquivalentTo(0))
})
})

Expand All @@ -695,7 +743,7 @@ b: bar
]
}`)}

err := ChangeRoot(&to, "/items", true)
err := ChangeRoot(&to, "/items", false, true)
if err != nil {
Fail(err.Error())
}
Expand Down
Loading

0 comments on commit b2a2e36

Please sign in to comment.