refactored and added test cases

cli/cmd/apply.go

@@ -3,6 +3,7 @@ package cmd
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"net/url"
@@ -142,21 +143,16 @@ func apply(_ *types.GetAuthenticatedUserResponse, client *api.Client, _ []string
 
 	worker.SetDefaultDriver("deploy")
 
-	if hasDeploymentHookEnvVars() {
-		deplNamespace := os.Getenv("PORTER_NAMESPACE")
-
-		if deplNamespace == "" {
-			return fmt.Errorf("namespace must be set by PORTER_NAMESPACE")
-		}
-
-		deploymentHook, err := NewDeploymentHook(client, resGroup, deplNamespace)
-
-		if err != nil {
-			return fmt.Errorf("error creating deployment hook: %w", err)
-		}
-
-		worker.RegisterHook("deployment", deploymentHook)
+	deploymentHookConfig := DeploymentHookConfig{
+		PorterAPIClient: client,
+		ResourceGroup:   resGroup,
 	}
+	deploymentHook, err := NewDeploymentHook(deploymentHookConfig)
+	if err != nil {
+		return fmt.Errorf("error creating deployment hook: %w", err)
+	}
+
+	worker.RegisterHook("deployment", &deploymentHook)
 
 	cloneEnvGroupHook := NewCloneEnvGroupHook(client, resGroup)
 	worker.RegisterHook("cloneenvgroup", cloneEnvGroupHook)
@@ -188,40 +184,80 @@ func applyValidate() error {
 	return nil
 }
 
-func hasDeploymentHookEnvVars() bool {
-	if ghIDStr := os.Getenv("PORTER_GIT_INSTALLATION_ID"); ghIDStr == "" {
-		return false
+// parseDeploymentHookEnvVars will check if an apply has the appropriate deployment hook environment variables
+// and add them to the provided config if present.
+// Any supplied values in the DeploymentHookConfig will take precedence over the environment variables
+func parseDeploymentHookEnvVars(conf *DeploymentHookConfig) error {
+	errResp := func(err error, key string) error {
+		return fmt.Errorf("unable to parse required environment variable: %s - %w", key, err)
+	}
+
+	if conf.GithubAppID == 0 {
+		ghIDStr := os.Getenv("PORTER_GIT_INSTALLATION_ID")
+		ghID, err := strconv.Atoi(ghIDStr)
+		if err != nil {
+			return errResp(err, "PORTER_GIT_INSTALLATION_ID")
+		}
+		conf.GithubAppID = ghID
+	}
+
+	if conf.BranchFrom == "" {
+		conf.BranchFrom = os.Getenv("PORTER_BRANCH_FROM")
+		if conf.BranchFrom == "" {
+			return errResp(errors.New("required parameter missing"), "PORTER_BRANCH_FROM")
+		}
 	}
 
-	if prIDStr := os.Getenv("PORTER_PULL_REQUEST_ID"); prIDStr == "" {
-		return false
+	if conf.BranchInto == "" {
+		conf.BranchInto = os.Getenv("PORTER_BRANCH_INTO")
 	}
 
-	if branchFrom := os.Getenv("PORTER_BRANCH_FROM"); branchFrom == "" {
-		return false
+	if conf.GithubActionID == 0 {
+		actionIDStr := os.Getenv("PORTER_ACTION_ID")
+		actionID, err := strconv.Atoi(actionIDStr)
+		if err != nil {
+			return errResp(err, "PORTER_ACTION_ID")
+		}
+		conf.GithubActionID = actionID
 	}
 
-	if branchInto := os.Getenv("PORTER_BRANCH_INTO"); branchInto == "" {
-		return false
+	if conf.RepoName == "" {
+		conf.RepoName = os.Getenv("PORTER_REPO_NAME")
+		if conf.RepoName == "" {
+			return errResp(errors.New("required parameter missing"), "PORTER_REPO_NAME")
+		}
 	}
 
-	if actionIDStr := os.Getenv("PORTER_ACTION_ID"); actionIDStr == "" {
-		return false
+	if conf.RepoOwner == "" {
+		conf.RepoOwner = os.Getenv("PORTER_REPO_OWNER")
+		if conf.RepoOwner == "" {
+			return errResp(errors.New("required parameter missing"), "PORTER_REPO_OWNER")
+		}
 	}
 
-	if repoName := os.Getenv("PORTER_REPO_NAME"); repoName == "" {
-		return false
+	if conf.PullRequestID == 0 {
+		prIDStr := os.Getenv("PORTER_PULL_REQUEST_ID")
+		prID, err := strconv.Atoi(prIDStr)
+		if err != nil {
+			return errResp(err, "PORTER_PULL_REQUEST_ID")
+		}
+		conf.PullRequestID = prID
 	}
 
-	if repoOwner := os.Getenv("PORTER_REPO_OWNER"); repoOwner == "" {
-		return false
+	if conf.PullRequestName == "" {
+		conf.PullRequestName = os.Getenv("PORTER_PR_NAME")
 	}
 
-	if prName := os.Getenv("PORTER_PR_NAME"); prName == "" {
-		return false
+	if conf.Namespace == "" {
+		conf.Namespace = os.Getenv("PORTER_NAMESPACE")
+		if conf.Namespace == "" {
+			conf.Namespace = preview.DefaultPreviewEnvironmentNamespace(conf.BranchFrom, conf.RepoOwner, conf.RepoName)
+		}
+		// setting namespace env var is required here as we rely on it globally. This can be removed when preview.getNamespace() is no longer used.
+		os.Setenv("PORTER_NAMESPACE", conf.Namespace)
 	}
 
-	return true
+	return nil
 }
 
 type DeployDriver struct {
@@ -675,6 +711,7 @@ func (d *DeployDriver) getAddonConfig(resource *switchboardModels.Resource) (map
 	})
 }
 
+// DeploymentHook is used for deploying an application into a given cluster
 type DeploymentHook struct {
 	client                                                                    *api.Client
 	resourceGroup                                                             *switchboardTypes.ResourceGroup
@@ -682,75 +719,80 @@ type DeploymentHook struct {
 	branchFrom, branchInto, namespace, repoName, repoOwner, prName, commitSHA string
 }
 
-func NewDeploymentHook(client *api.Client, resourceGroup *switchboardTypes.ResourceGroup, namespace string) (*DeploymentHook, error) {
-	res := &DeploymentHook{
-		client:        client,
-		resourceGroup: resourceGroup,
-		namespace:     namespace,
-	}
-
-	ghIDStr := os.Getenv("PORTER_GIT_INSTALLATION_ID")
-	ghID, err := strconv.Atoi(ghIDStr)
-
-	if err != nil {
-		return nil, err
-	}
-
-	res.gitInstallationID = uint(ghID)
-
-	prIDStr := os.Getenv("PORTER_PULL_REQUEST_ID")
-	prID, err := strconv.Atoi(prIDStr)
+// DeploymentHookConfig contains all config and overrides for initialising a deployment DeploymentHook
+// which is used for deploying a given `porter apply`
+type DeploymentHookConfig struct {
+	// ProjectID is the Porter Project ID for the cluster
+	ProjectID int
+	// ClusterID is the Porter Cluster ID for deploying into
+	ClusterID int
+	// PorterAPIClient is the settings used for communicating with the Porter API
+	PorterAPIClient *api.Client
+	// ResourceGroup ...
+	ResourceGroup *switchboardTypes.ResourceGroup
+	// Namespace is the name of the namespace into which an application will be deployed.
+	// If this is not provided, a default namespace will be used.
+	// In the case of preview environments, this will be previewbranch-XXX, where XXX is your PR name
+	Namespace string
+
+	// BranchFrom is the branch from which the preview environment will be created
+	BranchFrom string
+	// BranchInto is the branch in with a PR will be created. If using branch based preview environments,
+	// set this value to the same as BranchFrom
+	BranchInto string
+
+	// GithubID is the ID of the Github App used for deployment
+	GithubAppID int
+	// GithubActionID is the ID of the Github Action used to deploy an application
+	GithubActionID int
+	// PullRequestName is the name of a PR which will be used in the Preview Environment workflows
+	PullRequestName string
+	// PullRequestID is the ID of a PR which will be used in the Preview Environment workflows
+	PullRequestID int
+	// RepoName is the name of the given repository for deploying. In Github, with will be of the format <RepoOwner/RepoName> .i.e porter-dev/porter
+	RepoName string
+	// RepoOwner is the owner of the given repository for deploying. In Github, with will be of the format <RepoOwner/RepoName> .i.e porter-dev/porter
+	RepoOwner string
+}
 
+// NewDeploymentHook is used for creating a new DeploymentHook, checking that the required variable combinations are present
+func NewDeploymentHook(conf DeploymentHookConfig) (DeploymentHook, error) {
+	err := parseDeploymentHookEnvVars(&conf)
 	if err != nil {
-		return nil, err
+		return DeploymentHook{}, err
 	}
 
-	res.prID = uint(prID)
+	res := DeploymentHook{
+		client:        conf.PorterAPIClient,
+		resourceGroup: conf.ResourceGroup,
+		namespace:     conf.Namespace,
 
-	res.projectID = cliConf.Project
-
-	if res.projectID == 0 {
-		return nil, fmt.Errorf("project id must be set")
-	}
+		projectID: uint(conf.ProjectID),
+		clusterID: uint(conf.ClusterID),
 
-	res.clusterID = cliConf.Cluster
+		branchFrom: conf.BranchFrom,
+		branchInto: conf.BranchInto,
 
-	if res.clusterID == 0 {
-		return nil, fmt.Errorf("cluster id must be set")
-	}
-
-	branchFrom := os.Getenv("PORTER_BRANCH_FROM")
-	res.branchFrom = branchFrom
-
-	branchInto := os.Getenv("PORTER_BRANCH_INTO")
-	res.branchInto = branchInto
-
-	actionIDStr := os.Getenv("PORTER_ACTION_ID")
-	actionID, err := strconv.Atoi(actionIDStr)
-
-	if err != nil {
-		return nil, err
+		gitInstallationID: uint(conf.GithubAppID),
+		actionID:          uint(conf.GithubActionID),
+		repoName:          conf.RepoName,
+		repoOwner:         conf.RepoOwner,
 	}
 
-	res.actionID = uint(actionID)
-
-	repoName := os.Getenv("PORTER_REPO_NAME")
-	res.repoName = repoName
-
-	repoOwner := os.Getenv("PORTER_REPO_OWNER")
-	res.repoOwner = repoOwner
-
-	prName := os.Getenv("PORTER_PR_NAME")
-	res.prName = prName
-
 	commit, err := git.LastCommit()
-
 	if err != nil {
-		return nil, fmt.Errorf(err.Error())
+		return res, fmt.Errorf(err.Error())
 	}
-
 	res.commitSHA = commit.Sha[:7]
 
+	if !res.isBranchDeploy() {
+		if conf.PullRequestID == 0 || conf.PullRequestName == "" {
+			if err != nil {
+				return res, errors.New("PR based deploys require a PullRequestID and PullRequestName")
+			}
+		}
+	}
+
 	return res, nil
 }
 
@@ -788,14 +830,9 @@ func (t *DeploymentHook) PreApply() error {
 		return fmt.Errorf("could not find environment for deployment")
 	}
 
-	if t.isBranchDeploy() {
-		t.namespace = preview.GetNamespaceForBranchDeploy(t.branchFrom, t.repoOwner, t.repoName)
-	}
-
 	nsList, err := t.client.GetK8sNamespaces(
 		context.Background(), t.projectID, t.clusterID,
 	)
-
 	if err != nil {
 		return fmt.Errorf("error fetching namespaces: %w", err)
 	}

cli/cmd/apply_test.go

@@ -0,0 +1,67 @@
+package cmd
+
+import (
+	"net/http"
+	"os"
+	"testing"
+
+	"github.com/matryer/is"
+	"github.com/porter-dev/porter/api/client"
+	switchboardTypes "github.com/porter-dev/switchboard/pkg/types"
+)
+
+func Test_Apply(t *testing.T) {
+	tests := []struct {
+		name string
+		run  func(*testing.T, DeploymentHookConfig)
+	}{
+		{name: "preapply namespace defaults due to missing namespace env var", run: testPreApply_DeploymentHook_NamespaceDefaultWithEnvVar},
+		{name: "preapply namespace overrides to provided namespace from env var", run: testPreApply_DeploymentHook_NamespaceOverrideWithEnvVar},
+	}
+	for _, tc := range tests {
+		cli := client.Client{
+			BaseURL:    "localhost",
+			HTTPClient: http.DefaultClient,
+		}
+		conf := DeploymentHookConfig{
+			PorterAPIClient: &cli,
+			ResourceGroup:   &switchboardTypes.ResourceGroup{},
+			GithubAppID:     -1,
+			PullRequestID:   -1,
+			GithubActionID:  -1,
+		}
+		tc.run(t, conf)
+	}
+
+}
+
+func testPreApply_DeploymentHook_NamespaceDefaultWithEnvVar(t *testing.T, conf DeploymentHookConfig) {
+	is := is.New(t)
+
+	os.Setenv("PORTER_BRANCH_FROM", "testbranch")
+	os.Setenv("PORTER_BRANCH_INTO", "testbranch")
+	os.Setenv("PORTER_REPO_OWNER", "testowner")
+	os.Setenv("PORTER_REPO_NAME", "testname")
+
+	dh, err := NewDeploymentHook(conf)
+	is.NoErr(err) // no intended errors for setting up deployment hook
+
+	expectedNamespace := "previewbranch-testbranch-testowner-testname"
+	is.Equal(expectedNamespace, dh.namespace) // namespace should be generated based on provided env vars
+}
+
+func testPreApply_DeploymentHook_NamespaceOverrideWithEnvVar(t *testing.T, conf DeploymentHookConfig) {
+	is := is.New(t)
+
+	conf.BranchFrom = "anything"
+	conf.RepoName = "anything"
+	conf.RepoOwner = "anything"
+
+	customNamespace := "custom-namespace"
+	os.Setenv("PORTER_NAMESPACE", customNamespace)
+
+	dh, err := NewDeploymentHook(conf)
+	is.NoErr(err) // no intended errors for setting up deployment hook
+
+	is.Equal(customNamespace, dh.namespace) // namespace should be overridden entirely
+}

cli/cmd/preview/utils.go

@@ -192,7 +192,7 @@ func GetTarget(resourceName string, input map[string]interface{}) (*preview.Targ
 	return output, nil
 }
 
-func GetNamespaceForBranchDeploy(branch, owner, name string) string {
+func DefaultPreviewEnvironmentNamespace(branch, owner, name string) string {
 	namespace := fmt.Sprintf("previewbranch-%s-%s-%s", branch,
 		strings.ReplaceAll(strings.ToLower(owner), "_", "-"),
 		strings.ReplaceAll(strings.ToLower(name), "_", "-"))
@@ -210,7 +210,7 @@ func getNamespace() string {
 			if branchFrom, ok := os.LookupEnv("PORTER_BRANCH_FROM"); ok {
 				if branchInto, ok := os.LookupEnv("PORTER_BRANCH_INTO"); ok {
 					if branchInto == branchFrom { // branch deploy
-						return GetNamespaceForBranchDeploy(branchInto, owner, repo)
+						return DefaultPreviewEnvironmentNamespace(branchInto, owner, repo)
 					}
 				}
 			}

go.mod

@@ -71,6 +71,7 @@ require (
 	github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/containerregistry/armcontainerregistry v0.5.0
 	github.com/briandowns/spinner v1.18.1
 	github.com/glebarez/sqlite v1.6.0
+	github.com/matryer/is v1.4.0
 	github.com/open-policy-agent/opa v0.44.0
 	github.com/santhosh-tekuri/jsonschema/v5 v5.0.1
 	github.com/stefanmcshane/helm v0.0.0-20221213002717-88a4a2c6e77d

go.sum

@@ -1204,6 +1204,7 @@ github.com/markbates/safe v1.0.1 h1:yjZkbvRM6IzKj9tlu/zMJLS0n/V351OZWRnF3QfaUxI=
 github.com/markbates/safe v1.0.1/go.mod h1:nAqgmRi7cY2nqMc92/bSEeQA+R4OheNU2T1kNSCBdG0=
 github.com/marstr/guid v1.1.0/go.mod h1:74gB1z2wpxxInTG6yaqA7KrtM0NZ+RbrcqDvYHefzho=
 github.com/matoous/godox v0.0.0-20210227103229-6504466cf951/go.mod h1:1BELzlh859Sh1c6+90blK8lbYy0kwQf1bYlBhBysy1s=
+github.com/matryer/is v1.4.0 h1:sosSmIWwkYITGrxZ25ULNDeKiMNzFSr4V/eqBQP0PeE=
 github.com/matryer/is v1.4.0/go.mod h1:8I/i5uYgLzgsgEloJE1U6xx5HkBQpAZvepWuujKwMRU=
 github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaOChaDxuIBZU=
 github.com/mattn/go-colorable v0.1.1/go.mod h1:FuOcm+DKB9mbwrcAfNl7/TZVBZ6rcnceauSikq3lYCQ=