Merge pull request #2648 from priyawadhwa/constructor

Add constructor for creating portForwardEntry
GoogleContainerTools · Aug 14, 2019 · 443bcdc · 443bcdc
2 parents 686e75f + 45a4416
commit 443bcdc
Show file tree

Hide file tree

Showing 7 changed files with 54 additions and 79 deletions.
diff --git a/pkg/skaffold/kubernetes/portforward/entry_manager_test.go b/pkg/skaffold/kubernetes/portforward/entry_manager_test.go
@@ -48,22 +48,17 @@ func TestNewEntryManager(t *testing.T) {
 }
 
 func TestStop(t *testing.T) {
-	pfe1 := &portForwardEntry{
-		resource: latest.PortForwardResource{
-			Type:      constants.Pod,
-			Name:      "resource",
-			Namespace: "default",
-		},
-		localPort: 9000,
-	}
-	pfe2 := &portForwardEntry{
-		resource: latest.PortForwardResource{
-			Type:      constants.Pod,
-			Name:      "resource2",
-			Namespace: "default",
-		},
-		localPort: 9001,
-	}
+	pfe1 := newPortForwardEntry(0, latest.PortForwardResource{
+		Type:      constants.Pod,
+		Name:      "resource",
+		Namespace: "default",
+	}, "", "", "", 9000, false)
+
+	pfe2 := newPortForwardEntry(0, latest.PortForwardResource{
+		Type:      constants.Pod,
+		Name:      "resource2",
+		Namespace: "default",
+	}, "", "", "", 9001, false)
 
 	em := NewEntryManager(ioutil.Discard, nil)
 

diff --git a/pkg/skaffold/kubernetes/portforward/pod_forwarder.go b/pkg/skaffold/kubernetes/portforward/pod_forwarder.go
@@ -19,7 +19,6 @@ package portforward
 import (
 	"context"
 	"strconv"
-	"sync"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/color"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
@@ -138,15 +137,7 @@ func (p *WatchingPodForwarder) podForwardingEntry(resourceVersion, containerName
 	if err != nil {
 		return nil, errors.Wrap(err, "converting resource version to integer")
 	}
-	entry := &portForwardEntry{
-		resource:               resource,
-		resourceVersion:        rv,
-		podName:                resource.Name,
-		containerName:          containerName,
-		portName:               portName,
-		automaticPodForwarding: true,
-		terminationLock:        &sync.Mutex{},
-	}
+	entry := newPortForwardEntry(rv, resource, resource.Name, containerName, portName, 0, true)
 
 	// If we have, return the current entry
 	oldEntry, ok := p.forwardedResources.Load(entry.key())

diff --git a/pkg/skaffold/kubernetes/portforward/port_forward_entry.go b/pkg/skaffold/kubernetes/portforward/port_forward_entry.go
@@ -37,6 +37,20 @@ type portForwardEntry struct {
 	cancel                 context.CancelFunc
 }
 
+// newPortForwardEntry returns a port forward entry.
+func newPortForwardEntry(resourceVersion int, resource latest.PortForwardResource, podName, containerName, portName string, localPort int, automaticPodForwarding bool) *portForwardEntry {
+	return &portForwardEntry{
+		resourceVersion:        resourceVersion,
+		resource:               resource,
+		podName:                podName,
+		containerName:          containerName,
+		portName:               portName,
+		localPort:              localPort,
+		automaticPodForwarding: automaticPodForwarding,
+		terminationLock:        &sync.Mutex{},
+	}
+}
+
 // key is an identifier for the lock on a port during the skaffold dev cycle.
 // if automaticPodForwarding is set, we return a key that doesn't include podName, since we want the key
 // to be the same whenever pods restart

diff --git a/pkg/skaffold/kubernetes/portforward/port_forward_entry_test.go b/pkg/skaffold/kubernetes/portforward/port_forward_entry_test.go
@@ -32,25 +32,21 @@ func TestPortForwardEntryKey(t *testing.T) {
 	}{
 		{
 			description: "entry for pod",
-			pfe: &portForwardEntry{
-				resource: latest.PortForwardResource{
-					Type:      "pod",
-					Name:      "podName",
-					Namespace: "default",
-					Port:      8080,
-				},
-			},
+			pfe: newPortForwardEntry(0, latest.PortForwardResource{
+				Type:      "pod",
+				Name:      "podName",
+				Namespace: "default",
+				Port:      8080,
+			}, "", "", "", 0, false),
 			expected: "pod-podName-default-8080",
 		}, {
 			description: "entry for deploy",
-			pfe: &portForwardEntry{
-				resource: latest.PortForwardResource{
-					Type:      "deployment",
-					Name:      "depName",
-					Namespace: "namespace",
-					Port:      9000,
-				},
-			},
+			pfe: newPortForwardEntry(0, latest.PortForwardResource{
+				Type:      "deployment",
+				Name:      "depName",
+				Namespace: "namespace",
+				Port:      9000,
+			}, "", "", "", 0, false),
 			expected: "deployment-depName-namespace-9000",
 		},
 	}
@@ -78,17 +74,12 @@ func TestAutomaticPodForwardingKey(t *testing.T) {
 	}{
 		{
 			description: "entry for automatically port forwarded pod",
-			pfe: &portForwardEntry{
-				containerName: "containerName",
-				portName:      "portName",
-				resource: latest.PortForwardResource{
-					Type:      "pod",
-					Name:      "podName",
-					Namespace: "default",
-					Port:      8080,
-				},
-				automaticPodForwarding: true,
-			},
+			pfe: newPortForwardEntry(0, latest.PortForwardResource{
+				Type:      "pod",
+				Name:      "podName",
+				Namespace: "default",
+				Port:      8080,
+			}, "", "containerName", "portName", 0, true),
 			expected: "containerName-default-portName-8080",
 		},
 	}

diff --git a/pkg/skaffold/kubernetes/portforward/port_forward_integration.go b/pkg/skaffold/kubernetes/portforward/port_forward_integration.go
@@ -19,7 +19,6 @@ package portforward
 import (
 	"context"
 	"os"
-	"sync"
 	"testing"
 	"time"
 
@@ -38,18 +37,12 @@ func WhiteBoxPortForwardCycle(t *testing.T, kubectlCLI *kubectl.CLI, namespace s
 	portForwardEvent = func(entry *portForwardEntry) {}
 	ctx := context.Background()
 	localPort := retrieveAvailablePort(9000, em.forwardedPorts)
-	pfe := &portForwardEntry{
-		resource: latest.PortForwardResource{
-			Type:      "deployment",
-			Name:      "leeroy-web",
-			Namespace: namespace,
-			Port:      8080,
-		},
-		containerName:   "dummy container",
-		localPort:       localPort,
-		terminationLock: &sync.Mutex{},
-	}
-
+	pfe := newPortForwardEntry(0, latest.PortForwardResource{
+		Type:      "deployment",
+		Name:      "leeroy-web",
+		Namespace: namespace,
+		Port:      8080,
+	}, "", "dummy container", "", localPort, false)
 	defer em.Stop()
 	em.forwardPortForwardEntry(ctx, pfe)
 	em.Stop()

diff --git a/pkg/skaffold/kubernetes/portforward/resource_forwarder.go b/pkg/skaffold/kubernetes/portforward/resource_forwarder.go
@@ -18,7 +18,6 @@ package portforward
 
 import (
 	"context"
-	"sync"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes"
@@ -81,10 +80,8 @@ func (p *ResourceForwarder) portForwardResource(ctx context.Context, resource la
 
 func (p *ResourceForwarder) getCurrentEntry(resource latest.PortForwardResource) *portForwardEntry {
 	// determine if we have seen this before
-	entry := &portForwardEntry{
-		resource:        resource,
-		terminationLock: &sync.Mutex{},
-	}
+	entry := newPortForwardEntry(0, resource, "", "", "", 0, false)
+
 	// If we have, return the current entry
 	oldEntry, ok := p.forwardedResources.Load(entry.key())
 

diff --git a/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go b/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go
@@ -154,10 +154,7 @@ func TestGetCurrentEntryFunc(t *testing.T) {
 				Port: 8080,
 			},
 			availablePorts: []int{8080},
-			expected: &portForwardEntry{
-				localPort:       8080,
-				terminationLock: &sync.Mutex{},
-			},
+			expected:       newPortForwardEntry(0, latest.PortForwardResource{}, "", "", "", 8080, false),
 		}, {
 			description: "port forward existing deployment",
 			resource: latest.PortForwardResource{
@@ -177,10 +174,7 @@ func TestGetCurrentEntryFunc(t *testing.T) {
 					localPort: 9000,
 				},
 			},
-			expected: &portForwardEntry{
-				localPort:       9000,
-				terminationLock: &sync.Mutex{},
-			},
+			expected: newPortForwardEntry(0, latest.PortForwardResource{}, "", "", "", 9000, false),
 		},
 	}