From 6e503cc79bbe5c084e85e6b95eccfba0149402ae Mon Sep 17 00:00:00 2001 From: James Shubin Date: Wed, 31 May 2017 16:42:29 -0400 Subject: [PATCH] resources: Simplify the resource Compare functions This removes one level of indentation and simplifies the code. --- docs/resource-guide.md | 37 ++++++------ resources/augeas.go | 23 ++++---- resources/exec.go | 66 +++++++++++----------- resources/file.go | 70 +++++++++++------------ resources/hostname.go | 41 +++++++------- resources/kv.go | 53 +++++++++--------- resources/msg.go | 45 +++++++-------- resources/noop.go | 25 ++++----- resources/nspawn.go | 29 +++++----- resources/password.go | 47 ++++++++-------- resources/pkg.go | 48 ++++++++-------- resources/resources_test.go | 42 +++++++++++++- resources/svc.go | 42 +++++++------- resources/timer.go | 29 +++++----- resources/virt.go | 108 ++++++++++++++++++------------------ 15 files changed, 373 insertions(+), 332 deletions(-) diff --git a/docs/resource-guide.md b/docs/resource-guide.md index 11fc245b..79d858c2 100644 --- a/docs/resource-guide.md +++ b/docs/resource-guide.md @@ -344,25 +344,26 @@ some way. #### Example ```golang // Compare two resources and return if they are equivalent. -func (obj *FooRes) Compare(res Res) bool { - switch res.(type) { - case *FooRes: // only compare to other resources of the Foo kind! - res := res.(*FileRes) - if !obj.BaseRes.Compare(res) { // call base Compare - return false - } - if obj.Name != res.Name { - return false - } - if obj.whatever != res.whatever { - return false - } - if obj.Flag != res.Flag { - return false - } - default: - return false // different kind of resource +func (obj *FooRes) Compare(r Res) bool { + // we can only compare FooRes to others of the same resource kind + res, ok := r.(*FooRes) + if !ok { + return false } + if !obj.BaseRes.Compare(res) { // call base Compare + return false + } + if obj.Name != res.Name { + return false + } + + if obj.whatever != res.whatever { + return false + } + if obj.Flag != res.Flag { + return false + } + return true // they must match! } ``` diff --git a/resources/augeas.go b/resources/augeas.go index 398e46a8..ccace8be 100644 --- a/resources/augeas.go +++ b/resources/augeas.go @@ -268,20 +268,19 @@ func (obj *AugeasRes) GroupCmp(r Res) bool { } // Compare two resources and return if they are equivalent. -func (obj *AugeasRes) Compare(res Res) bool { - switch res.(type) { - // we can only compare AugeasRes to others of the same resource - case *AugeasRes: - res := res.(*AugeasRes) - if !obj.BaseRes.Compare(res) { // call base Compare - return false - } - if obj.Name != res.Name { - return false - } - default: +func (obj *AugeasRes) Compare(r Res) bool { + // we can only compare AugeasRes to others of the same resource kind + res, ok := r.(*AugeasRes) + if !ok { return false } + if !obj.BaseRes.Compare(res) { // call base Compare + return false + } + if obj.Name != res.Name { + return false + } + return true } diff --git a/resources/exec.go b/resources/exec.go index 2af58766..99b0698a 100644 --- a/resources/exec.go +++ b/resources/exec.go @@ -340,41 +340,41 @@ func (obj *ExecRes) GroupCmp(r Res) bool { } // Compare two resources and return if they are equivalent. -func (obj *ExecRes) Compare(res Res) bool { - switch res.(type) { - case *ExecRes: - res := res.(*ExecRes) - if !obj.BaseRes.Compare(res) { // call base Compare - return false - } - - if obj.Name != res.Name { - return false - } - if obj.Cmd != res.Cmd { - return false - } - if obj.Shell != res.Shell { - return false - } - if obj.Timeout != res.Timeout { - return false - } - if obj.WatchCmd != res.WatchCmd { - return false - } - if obj.WatchShell != res.WatchShell { - return false - } - if obj.IfCmd != res.IfCmd { - return false - } - if obj.IfShell != res.IfShell { - return false - } - default: +func (obj *ExecRes) Compare(r Res) bool { + // we can only compare ExecRes to others of the same resource kind + res, ok := r.(*ExecRes) + if !ok { return false } + if !obj.BaseRes.Compare(res) { // call base Compare + return false + } + if obj.Name != res.Name { + return false + } + + if obj.Cmd != res.Cmd { + return false + } + if obj.Shell != res.Shell { + return false + } + if obj.Timeout != res.Timeout { + return false + } + if obj.WatchCmd != res.WatchCmd { + return false + } + if obj.WatchShell != res.WatchShell { + return false + } + if obj.IfCmd != res.IfCmd { + return false + } + if obj.IfShell != res.IfShell { + return false + } + return true } diff --git a/resources/file.go b/resources/file.go index 7a1785f3..01d578d5 100644 --- a/resources/file.go +++ b/resources/file.go @@ -942,43 +942,43 @@ func (obj *FileRes) GroupCmp(r Res) bool { } // Compare two resources and return if they are equivalent. -func (obj *FileRes) Compare(res Res) bool { - switch res.(type) { - case *FileRes: - res := res.(*FileRes) - if !obj.BaseRes.Compare(res) { // call base Compare - return false - } - - if obj.Name != res.Name { - return false - } - if obj.path != res.path { - return false - } - if (obj.Content == nil) != (res.Content == nil) { // xor - return false - } - if obj.Content != nil && res.Content != nil { - if *obj.Content != *res.Content { // compare the strings - return false - } - } - if obj.Source != res.Source { - return false - } - if obj.State != res.State { - return false - } - if obj.Recurse != res.Recurse { - return false - } - if obj.Force != res.Force { - return false - } - default: +func (obj *FileRes) Compare(r Res) bool { + // we can only compare FileRes to others of the same resource kind + res, ok := r.(*FileRes) + if !ok { return false } + if !obj.BaseRes.Compare(res) { // call base Compare + return false + } + if obj.Name != res.Name { + return false + } + + if obj.path != res.path { + return false + } + if (obj.Content == nil) != (res.Content == nil) { // xor + return false + } + if obj.Content != nil && res.Content != nil { + if *obj.Content != *res.Content { // compare the strings + return false + } + } + if obj.Source != res.Source { + return false + } + if obj.State != res.State { + return false + } + if obj.Recurse != res.Recurse { + return false + } + if obj.Force != res.Force { + return false + } + return true } diff --git a/resources/hostname.go b/resources/hostname.go index 1d295937..54af02c9 100644 --- a/resources/hostname.go +++ b/resources/hostname.go @@ -252,28 +252,29 @@ func (obj *HostnameRes) GroupCmp(r Res) bool { } // Compare two resources and return if they are equivalent. -func (obj *HostnameRes) Compare(res Res) bool { - switch res := res.(type) { - // we can only compare HostnameRes to others of the same resource - case *HostnameRes: - if !obj.BaseRes.Compare(res) { // call base Compare - return false - } - if obj.Name != res.Name { - return false - } - if obj.PrettyHostname != res.PrettyHostname { - return false - } - if obj.StaticHostname != res.StaticHostname { - return false - } - if obj.TransientHostname != res.TransientHostname { - return false - } - default: +func (obj *HostnameRes) Compare(r Res) bool { + // we can only compare HostnameRes to others of the same resource kind + res, ok := r.(*HostnameRes) + if !ok { return false } + if !obj.BaseRes.Compare(res) { // call base Compare + return false + } + if obj.Name != res.Name { + return false + } + + if obj.PrettyHostname != res.PrettyHostname { + return false + } + if obj.StaticHostname != res.StaticHostname { + return false + } + if obj.TransientHostname != res.TransientHostname { + return false + } + return true } diff --git a/resources/kv.go b/resources/kv.go index bcf9f9db..d502df11 100644 --- a/resources/kv.go +++ b/resources/kv.go @@ -252,35 +252,34 @@ func (obj *KVRes) GroupCmp(r Res) bool { } // Compare two resources and return if they are equivalent. -func (obj *KVRes) Compare(res Res) bool { - switch res.(type) { - // we can only compare KVRes to others of the same resource - case *KVRes: - res := res.(*KVRes) - if !obj.BaseRes.Compare(res) { // call base Compare - return false - } - - if obj.Key != res.Key { - return false - } - if (obj.Value == nil) != (res.Value == nil) { // xor - return false - } - if obj.Value != nil && res.Value != nil { - if *obj.Value != *res.Value { // compare the strings - return false - } - } - if obj.SkipLessThan != res.SkipLessThan { - return false - } - if obj.SkipCmpStyle != res.SkipCmpStyle { - return false - } - default: +func (obj *KVRes) Compare(r Res) bool { + // we can only compare KVRes to others of the same resource kind + res, ok := r.(*KVRes) + if !ok { return false } + if !obj.BaseRes.Compare(res) { // call base Compare + return false + } + + if obj.Key != res.Key { + return false + } + if (obj.Value == nil) != (res.Value == nil) { // xor + return false + } + if obj.Value != nil && res.Value != nil { + if *obj.Value != *res.Value { // compare the strings + return false + } + } + if obj.SkipLessThan != res.SkipLessThan { + return false + } + if obj.SkipCmpStyle != res.SkipCmpStyle { + return false + } + return true } diff --git a/resources/msg.go b/resources/msg.go index 32142fc7..edee38cf 100644 --- a/resources/msg.go +++ b/resources/msg.go @@ -210,30 +210,31 @@ func (obj *MsgRes) AutoEdges() AutoEdge { } // Compare two resources and return if they are equivalent. -func (obj *MsgRes) Compare(res Res) bool { - switch res.(type) { - case *MsgRes: - res := res.(*MsgRes) - if !obj.BaseRes.Compare(res) { - return false - } - if obj.Body != res.Body { - return false - } - if obj.Priority != res.Priority { - return false - } - if len(obj.Fields) != len(res.Fields) { - return false - } - for field, value := range obj.Fields { - if res.Fields[field] != value { - return false - } - } - default: +func (obj *MsgRes) Compare(r Res) bool { + // we can only compare MsgRes to others of the same resource kind + res, ok := r.(*MsgRes) + if !ok { return false } + if !obj.BaseRes.Compare(res) { + return false + } + + if obj.Body != res.Body { + return false + } + if obj.Priority != res.Priority { + return false + } + if len(obj.Fields) != len(res.Fields) { + return false + } + for field, value := range obj.Fields { + if res.Fields[field] != value { + return false + } + } + return true } diff --git a/resources/noop.go b/resources/noop.go index 3e136e15..b676097d 100644 --- a/resources/noop.go +++ b/resources/noop.go @@ -123,21 +123,20 @@ func (obj *NoopRes) GroupCmp(r Res) bool { } // Compare two resources and return if they are equivalent. -func (obj *NoopRes) Compare(res Res) bool { - switch res.(type) { - // we can only compare NoopRes to others of the same resource - case *NoopRes: - res := res.(*NoopRes) - // calling base Compare is unneeded for the noop res - //if !obj.BaseRes.Compare(res) { // call base Compare - // return false - //} - if obj.Name != res.Name { - return false - } - default: +func (obj *NoopRes) Compare(r Res) bool { + // we can only compare NoopRes to others of the same resource kind + res, ok := r.(*NoopRes) + if !ok { return false } + // calling base Compare is probably unneeded for the noop res, but do it + if !obj.BaseRes.Compare(res) { // call base Compare + return false + } + if obj.Name != res.Name { + return false + } + return true } diff --git a/resources/nspawn.go b/resources/nspawn.go index 5d615d00..e55997aa 100644 --- a/resources/nspawn.go +++ b/resources/nspawn.go @@ -275,22 +275,23 @@ func (obj *NspawnRes) GroupCmp(r Res) bool { } // Compare two resources and return if they are equivalent. -func (obj *NspawnRes) Compare(res Res) bool { - switch res.(type) { - case *NspawnRes: - res := res.(*NspawnRes) - if !obj.BaseRes.Compare(res) { - return false - } - if obj.Name != res.Name { - return false - } - if !obj.svc.Compare(res.svc) { - return false - } - default: +func (obj *NspawnRes) Compare(r Res) bool { + // we can only compare NspawnRes to others of the same resource kind + res, ok := r.(*NspawnRes) + if !ok { return false } + if !obj.BaseRes.Compare(res) { + return false + } + if obj.Name != res.Name { + return false + } + + if !obj.svc.Compare(res.svc) { + return false + } + return true } diff --git a/resources/password.go b/resources/password.go index fc6ac781..2ffa02e1 100644 --- a/resources/password.go +++ b/resources/password.go @@ -322,32 +322,31 @@ func (obj *PasswordRes) GroupCmp(r Res) bool { } // Compare two resources and return if they are equivalent. -func (obj *PasswordRes) Compare(res Res) bool { - switch res.(type) { - // we can only compare PasswordRes to others of the same resource - case *PasswordRes: - res := res.(*PasswordRes) - if !obj.BaseRes.Compare(res) { // call base Compare - return false - } - - if obj.Name != res.Name { - return false - } - if obj.Length != res.Length { - return false - } - // TODO: we *could* optimize by allowing CheckApply to move from - // saved->!saved, by removing the file, but not likely worth it! - if obj.Saved != res.Saved { - return false - } - if obj.CheckRecovery != res.CheckRecovery { - return false - } - default: +func (obj *PasswordRes) Compare(r Res) bool { + // we can only compare PasswordRes to others of the same resource kind + res, ok := r.(*PasswordRes) + if !ok { return false } + if !obj.BaseRes.Compare(res) { // call base Compare + return false + } + if obj.Name != res.Name { + return false + } + + if obj.Length != res.Length { + return false + } + // TODO: we *could* optimize by allowing CheckApply to move from + // saved->!saved, by removing the file, but not likely worth it! + if obj.Saved != res.Saved { + return false + } + if obj.CheckRecovery != res.CheckRecovery { + return false + } + return true } diff --git a/resources/pkg.go b/resources/pkg.go index 3f9b403f..e68817a2 100644 --- a/resources/pkg.go +++ b/resources/pkg.go @@ -482,32 +482,32 @@ func (obj *PkgRes) GroupCmp(r Res) bool { } // Compare two resources and return if they are equivalent. -func (obj *PkgRes) Compare(res Res) bool { - switch res.(type) { - case *PkgRes: - res := res.(*PkgRes) - if !obj.BaseRes.Compare(res) { // call base Compare - return false - } - - if obj.Name != res.Name { - return false - } - if obj.State != res.State { - return false - } - if obj.AllowUntrusted != res.AllowUntrusted { - return false - } - if obj.AllowNonFree != res.AllowNonFree { - return false - } - if obj.AllowUnsupported != res.AllowUnsupported { - return false - } - default: +func (obj *PkgRes) Compare(r Res) bool { + // we can only compare PkgRes to others of the same resource kind + res, ok := r.(*PkgRes) + if !ok { return false } + if !obj.BaseRes.Compare(res) { // call base Compare + return false + } + if obj.Name != res.Name { + return false + } + + if obj.State != res.State { + return false + } + if obj.AllowUntrusted != res.AllowUntrusted { + return false + } + if obj.AllowNonFree != res.AllowNonFree { + return false + } + if obj.AllowUnsupported != res.AllowUnsupported { + return false + } + return true } diff --git a/resources/resources_test.go b/resources/resources_test.go index 7911e2df..1d9ddd45 100644 --- a/resources/resources_test.go +++ b/resources/resources_test.go @@ -22,9 +22,49 @@ import ( "encoding/base64" "encoding/gob" "testing" - //"github.com/purpleidea/mgmt/event" ) +func TestCompare1(t *testing.T) { + r1 := &NoopRes{} + r2 := &NoopRes{} + r3 := &FileRes{} + + if !r1.Compare(r2) || !r2.Compare(r1) { + t.Error("The two resources do not match!") + } + + if r1.Compare(r3) || r3.Compare(r1) { + t.Error("The two resources should not match!") + } +} + +func TestCompare2(t *testing.T) { + r1 := &NoopRes{ + BaseRes: BaseRes{ + Name: "noop1", + MetaParams: MetaParams{ + Noop: true, + }, + }, + } + r2 := &NoopRes{ + BaseRes: BaseRes{ + Name: "noop1", // same nampe + MetaParams: MetaParams{ + Noop: false, // different noop + }, + }, + } + + if !r2.Compare(r1) { // going from noop(false) -> noop(true) is okay! + t.Error("The two resources do not match!") + } + + if r1.Compare(r2) { // going from noop(true) -> noop(false) is not okay! + t.Error("The two resources should not match!") + } +} + func TestMiscEncodeDecode1(t *testing.T) { var err error //gob.Register( &NoopRes{} ) // happens in noop.go : init() diff --git a/resources/svc.go b/resources/svc.go index b9e7162a..6c8dc90a 100644 --- a/resources/svc.go +++ b/resources/svc.go @@ -427,29 +427,29 @@ func (obj *SvcRes) GroupCmp(r Res) bool { } // Compare two resources and return if they are equivalent. -func (obj *SvcRes) Compare(res Res) bool { - switch res.(type) { - case *SvcRes: - res := res.(*SvcRes) - if !obj.BaseRes.Compare(res) { // call base Compare - return false - } - - if obj.Name != res.Name { - return false - } - if obj.State != res.State { - return false - } - if obj.Startup != res.Startup { - return false - } - if obj.Session != res.Session { - return false - } - default: +func (obj *SvcRes) Compare(r Res) bool { + // we can only compare SvcRes to others of the same resource kind + res, ok := r.(*SvcRes) + if !ok { return false } + if !obj.BaseRes.Compare(res) { // call base Compare + return false + } + if obj.Name != res.Name { + return false + } + + if obj.State != res.State { + return false + } + if obj.Startup != res.Startup { + return false + } + if obj.Session != res.Session { + return false + } + return true } diff --git a/resources/timer.go b/resources/timer.go index 0c5597f1..985e1759 100644 --- a/resources/timer.go +++ b/resources/timer.go @@ -135,22 +135,23 @@ func (obj *TimerRes) AutoEdges() AutoEdge { } // Compare two resources and return if they are equivalent. -func (obj *TimerRes) Compare(res Res) bool { - switch res.(type) { - case *TimerRes: - res := res.(*TimerRes) - if !obj.BaseRes.Compare(res) { - return false - } - if obj.Name != res.Name { - return false - } - if obj.Interval != res.Interval { - return false - } - default: +func (obj *TimerRes) Compare(r Res) bool { + // we can only compare TimerRes to others of the same resource kind + res, ok := r.(*TimerRes) + if !ok { return false } + if !obj.BaseRes.Compare(res) { + return false + } + if obj.Name != res.Name { + return false + } + + if obj.Interval != res.Interval { + return false + } + return true } diff --git a/resources/virt.go b/resources/virt.go index 82918316..f8796485 100644 --- a/resources/virt.go +++ b/resources/virt.go @@ -1076,62 +1076,62 @@ func (obj *VirtRes) AutoEdges() AutoEdge { } // Compare two resources and return if they are equivalent. -func (obj *VirtRes) Compare(res Res) bool { - switch res.(type) { - case *VirtRes: - res := res.(*VirtRes) - if !obj.BaseRes.Compare(res) { // call base Compare - return false - } - - if obj.Name != res.Name { - return false - } - if obj.URI != res.URI { - return false - } - if obj.State != res.State { - return false - } - if obj.Transient != res.Transient { - return false - } - if obj.CPUs != res.CPUs { - return false - } - // we can't change this property while machine is running! - // we do need to return false, so that a new struct gets built, - // which will cause at least one Init() & CheckApply() to run. - if obj.MaxCPUs != res.MaxCPUs { - return false - } - // TODO: can we skip the compare of certain properties such as - // Memory because this object (but with different memory) can be - // *converted* into the new version that has more/less memory? - // We would need to run some sort of "old struct update", to get - // the new values, but that's easy to add. - if obj.Memory != res.Memory { - return false - } - // TODO: - //if obj.Boot != res.Boot { - // return false - //} - //if obj.Disk != res.Disk { - // return false - //} - //if obj.CDRom != res.CDRom { - // return false - //} - //if obj.Network != res.Network { - // return false - //} - //if obj.Filesystem != res.Filesystem { - // return false - //} - default: +func (obj *VirtRes) Compare(r Res) bool { + // we can only compare VirtRes to others of the same resource kind + res, ok := r.(*VirtRes) + if !ok { return false } + if !obj.BaseRes.Compare(res) { // call base Compare + return false + } + if obj.Name != res.Name { + return false + } + + if obj.URI != res.URI { + return false + } + if obj.State != res.State { + return false + } + if obj.Transient != res.Transient { + return false + } + if obj.CPUs != res.CPUs { + return false + } + // we can't change this property while machine is running! + // we do need to return false, so that a new struct gets built, + // which will cause at least one Init() & CheckApply() to run. + if obj.MaxCPUs != res.MaxCPUs { + return false + } + // TODO: can we skip the compare of certain properties such as + // Memory because this object (but with different memory) can be + // *converted* into the new version that has more/less memory? + // We would need to run some sort of "old struct update", to get + // the new values, but that's easy to add. + if obj.Memory != res.Memory { + return false + } + // TODO: + //if obj.Boot != res.Boot { + // return false + //} + //if obj.Disk != res.Disk { + // return false + //} + //if obj.CDRom != res.CDRom { + // return false + //} + //if obj.Network != res.Network { + // return false + //} + //if obj.Filesystem != res.Filesystem { + // return false + //} + return true }