Rethinking provider.Equals

We don't want the equality testing as is done here, but some sort of "kinda looks like" testing to make sure we don't add magic providers when a provider has been explicitly configured. So rename the method and loosen the comparisons.
seveas · Jul 24, 2020 · 256740a · 256740a
1 parent f48c4f3
commit 256740a
Show file tree

Hide file tree

Showing 13 changed files with 53 additions and 61 deletions.
diff --git a/aws_provider.go b/aws_provider.go
@@ -6,13 +6,13 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"reflect"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/credentials"
 	"github.com/aws/aws-sdk-go/aws/endpoints"
 	"github.com/aws/aws-sdk-go/aws/session"
 	"github.com/aws/aws-sdk-go/service/ec2"
-	"github.com/go-test/deep"
 	"github.com/spf13/viper"
 )
 
@@ -52,17 +52,16 @@ func NewAwsProvider(name string) HostProvider {
 	return p
 }
 
-func (p *AwsProvider) Equals(o HostProvider) bool {
+func (p *AwsProvider) Equivalent(o HostProvider) bool {
 	if c, ok := o.(*Cache); ok {
 		o = c.Source
 	}
 	op, ok := o.(*AwsProvider)
 	return ok &&
-		p.BaseProvider.Equals(&op.BaseProvider) &&
 		p.AccessKeyId == op.AccessKeyId &&
 		p.SecretAccessKey == op.SecretAccessKey &&
 		p.Partition == op.Partition &&
-		deep.Equal(p.Regions, op.Regions) == nil
+		reflect.DeepEqual(p.Regions, op.Regions)
 }
 
 func (p *AwsProvider) ParseViper(v *viper.Viper) error {

diff --git a/cache.go b/cache.go
@@ -31,13 +31,13 @@ func NewCacheFromProvider(p HostProvider) HostProvider {
 	}
 }
 
-func (c *Cache) Equals(p HostProvider) bool {
+func (c *Cache) Equivalent(p HostProvider) bool {
 	// We ignore the cache parameters, so caching doesn't actually change
 	// whether providers are equal.
 	if o, ok := p.(*Cache); ok {
 		p = o.Source
 	}
-	return c.Source.Equals(p)
+	return c.Source.Equivalent(p)
 }
 
 func (c *Cache) ParseViper(v *viper.Viper) error {

diff --git a/cache_test.go b/cache_test.go
@@ -16,7 +16,7 @@ type fakeProvider struct {
 	loaded       int
 }
 
-func (p *fakeProvider) Equals(o HostProvider) bool {
+func (p *fakeProvider) Equivalent(o HostProvider) bool {
 	return false
 }
 
@@ -60,13 +60,13 @@ func TestCache(t *testing.T) {
 	}
 }
 
-func TestCacheEqual(t *testing.T) {
+func TestCacheEquivalent(t *testing.T) {
 	p := NewPlainTextProvider("plain")
 	c := NewCacheFromProvider(p)
-	if !c.Equals(p) {
+	if !c.Equivalent(p) {
 		t.Errorf("Caching is affecting equality")
 	}
-	if !p.Equals(c) {
+	if !p.Equivalent(c) {
 		t.Errorf("Caching is affecting equality in reverse")
 	}
 }
diff --git a/consul_provider.go b/consul_provider.go
@@ -34,14 +34,12 @@ func NewConsulProvider(name string) HostProvider {
 	return &ConsulProvider{BaseProvider: BaseProvider{Name: name}, Address: addr}
 }
 
-func (p *ConsulProvider) Equals(o HostProvider) bool {
+func (p *ConsulProvider) Equivalent(o HostProvider) bool {
 	if c, ok := o.(*Cache); ok {
 		o = c.Source
 	}
 	op, ok := o.(*ConsulProvider)
-	return ok &&
-		p.BaseProvider.Equals(&op.BaseProvider) &&
-		p.Address == op.Address
+	return ok && p.Address == op.Address
 }
 
 func (p *ConsulProvider) ParseViper(v *viper.Viper) error {

diff --git a/consul_provider_test.go b/consul_provider_test.go
@@ -0,0 +1,23 @@
+package herd
+
+import (
+	"os"
+	"testing"
+)
+
+func TestDuplicateProviders(t *testing.T) {
+	r := NewRegistry("/nonexistent", "/nonexistent")
+	p := NewConsulProvider("test")
+	p.(*ConsulProvider).Address = "http://consul:8080"
+	r.AddProvider(p)
+	os.Setenv("CONSUL_HTTP_ADDR", "http://consul:8080")
+	r.AddMagicProvider(NewCacheFromProvider(NewConsulProvider("test 2")))
+	if len(r.providers) != 1 {
+		t.Errorf("AddMagicProviders did not detect duplicate consul provider")
+	}
+	os.Setenv("CONSUL_HTTP_ADDR", "http://consul:8081")
+	r.AddMagicProvider(NewCacheFromProvider(NewConsulProvider("test 3")))
+	if len(r.providers) != 2 {
+		t.Errorf("AddMagicProviders detected a duplicate provider where there is none")
+	}
+}
diff --git a/http_provider.go b/http_provider.go
@@ -6,9 +6,9 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net/http"
+	"reflect"
 	"time"
 
-	"github.com/go-test/deep"
 	"github.com/spf13/viper"
 )
 
@@ -24,17 +24,16 @@ func NewHttpProvider(name string) HostProvider {
 	return &HttpProvider{BaseProvider: BaseProvider{Name: name, Timeout: 30 * time.Second}}
 }
 
-func (p *HttpProvider) Equals(o HostProvider) bool {
+func (p *HttpProvider) Equivalent(o HostProvider) bool {
 	if c, ok := o.(*Cache); ok {
 		o = c.Source
 	}
 	op, ok := o.(*HttpProvider)
 	return ok &&
-		p.BaseProvider.Equals(&op.BaseProvider) &&
 		p.Url == op.Url &&
 		p.Username == op.Username &&
 		p.Password == op.Password &&
-		deep.Equal(p.Headers, op.Headers) == nil
+		reflect.DeepEqual(p.Headers, op.Headers)
 }
 
 func (p *HttpProvider) ParseViper(v *viper.Viper) error {

diff --git a/json_provider.go b/json_provider.go
@@ -18,14 +18,12 @@ func NewJsonProvider(name string) HostProvider {
 	return &JsonProvider{BaseProvider: BaseProvider{Name: name}}
 }
 
-func (p *JsonProvider) Equals(o HostProvider) bool {
+func (p *JsonProvider) Equivalent(o HostProvider) bool {
 	if c, ok := o.(*Cache); ok {
 		o = c.Source
 	}
 	op, ok := o.(*JsonProvider)
-	return ok &&
-		p.BaseProvider.Equals(&op.BaseProvider) &&
-		p.File == op.File
+	return ok && p.File == op.File
 }
 
 func (p *JsonProvider) ParseViper(v *viper.Viper) error {

diff --git a/known_hosts.go b/known_hosts.go
@@ -7,8 +7,8 @@ import (
 	"os"
 	"os/user"
 	"path/filepath"
+	"reflect"
 
-	"github.com/go-test/deep"
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/viper"
 	"golang.org/x/crypto/ssh"
@@ -32,14 +32,12 @@ func NewKnownHostsProvider(name string) HostProvider {
 	return &KnownHostsProvider{BaseProvider: BaseProvider{Name: name}, Files: files}
 }
 
-func (p *KnownHostsProvider) Equals(o HostProvider) bool {
+func (p *KnownHostsProvider) Equivalent(o HostProvider) bool {
 	if c, ok := o.(*Cache); ok {
 		o = c.Source
 	}
 	op, ok := o.(*KnownHostsProvider)
-	return ok &&
-		p.BaseProvider.Equals(&op.BaseProvider) &&
-		deep.Equal(p.Files, op.Files) == nil
+	return ok && reflect.DeepEqual(p.Files, op.Files)
 }
 
 func (p *KnownHostsProvider) ParseViper(v *viper.Viper) error {

diff --git a/prometheus_provider.go b/prometheus_provider.go
@@ -7,10 +7,10 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/url"
+	"reflect"
 	"strings"
 	"time"
 
-	"github.com/go-test/deep"
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/viper"
 )
@@ -43,14 +43,14 @@ func NewPrometheusProvider(name string) HostProvider {
 	return &PrometheusProvider{HttpProvider: HttpProvider{BaseProvider: BaseProvider{Name: name}}}
 }
 
-func (p *PrometheusProvider) Equals(o HostProvider) bool {
+func (p *PrometheusProvider) Equivalent(o HostProvider) bool {
 	if c, ok := o.(*Cache); ok {
 		o = c.Source
 	}
 	op, ok := o.(*PrometheusProvider)
 	return ok &&
-		p.HttpProvider.Equals(&op.HttpProvider) &&
-		deep.Equal(p.Jobs, op.Jobs) == nil
+		p.HttpProvider.Equivalent(&op.HttpProvider) &&
+		reflect.DeepEqual(p.Jobs, op.Jobs)
 }
 
 func (p *PrometheusProvider) ParseViper(v *viper.Viper) error {

diff --git a/putty_windows.go b/putty_windows.go
@@ -141,12 +141,12 @@ type PuttyProvider struct {
 	BaseProvider `mapstructure:",squash"`
 }
 
-func (p *PuttyProvider) Equals(o HostProvider) bool {
+func (p *PuttyProvider) Equivalent(o HostProvider) bool {
 	if c, ok := o.(*Cache); ok {
 		o = c.Source
 	}
 	op, ok := o.(*PuttyProvider)
-	return ok && p.BaseProvider.Equals(&op.BaseProvider)
+	return ok
 }
 
 func (p *PuttyProvider) ParseViper(v *viper.Viper) error {

diff --git a/registry.go b/registry.go
@@ -55,7 +55,7 @@ type Hosts []*Host
 type HostProvider interface {
 	ParseViper(v *viper.Viper) error
 	Load(ctx context.Context, mc chan CacheMessage) (Hosts, error)
-	Equals(p HostProvider) bool
+	Equivalent(p HostProvider) bool
 	base() *BaseProvider
 }
 
@@ -70,12 +70,6 @@ func (p *BaseProvider) base() *BaseProvider {
 	return p
 }
 
-func (p *BaseProvider) Equals(o *BaseProvider) bool {
-	return p.Name == o.Name &&
-		p.Prefix == o.Prefix &&
-		p.Timeout == o.Timeout
-}
-
 type CacheMessage struct {
 	Name     string
 	Finished bool
@@ -174,7 +168,7 @@ func (r *Registry) AddProvider(p HostProvider) {
 func (r *Registry) AddMagicProvider(p HostProvider) {
 	p.base().magic = true
 	for _, pr := range r.providers {
-		if pr.Equals(p) {
+		if pr.Equivalent(p) {
 			return
 		}
 	}

diff --git a/registry_test.go b/registry_test.go
@@ -2,7 +2,6 @@ package herd
 
 import (
 	"context"
-	"fmt"
 	"os"
 	"path/filepath"
 	"reflect"
@@ -47,7 +46,7 @@ type FakeProvider struct {
 	BaseProvider `mapstructure:",squash"`
 }
 
-func (p *FakeProvider) Equals(o HostProvider) bool {
+func (p *FakeProvider) Equivalent(o HostProvider) bool {
 	return false
 }
 
@@ -89,17 +88,3 @@ func TestRelativeFiles(t *testing.T) {
 		t.Errorf("Proper cache path not set, found %s", c2.File)
 	}
 }
-
-func TestBaseEquals(t *testing.T) {
-	for name, constructor := range availableProviders {
-		if name == "cache" {
-			continue
-		}
-		p1 := constructor(fmt.Sprintf("test-%s", name))
-		p2 := constructor(fmt.Sprintf("test-%s", name))
-		p2.base().Prefix = name + ":"
-		if p1.Equals(p2) {
-			t.Errorf("Provider %s is not testing base equality", name)
-		}
-	}
-}
diff --git a/simple_providers.go b/simple_providers.go
@@ -18,14 +18,12 @@ func NewPlainTextProvider(name string) HostProvider {
 	return &PlainTextProvider{BaseProvider: BaseProvider{Name: name}}
 }
 
-func (p *PlainTextProvider) Equals(o HostProvider) bool {
+func (p *PlainTextProvider) Equivalent(o HostProvider) bool {
 	if c, ok := o.(*Cache); ok {
 		o = c.Source
 	}
 	op, ok := o.(*PlainTextProvider)
-	return ok &&
-		p.BaseProvider.Equals(&op.BaseProvider) &&
-		p.File == op.File
+	return ok && p.File == op.File
 }
 
 func (p *PlainTextProvider) ParseViper(v *viper.Viper) error {