From 4b6b36c2ccdd6d9d335a4203b3744cf1c54414d9 Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Mon, 16 Jan 2023 15:16:21 +0900 Subject: [PATCH] env opaque box test --- client_int_test.go | 19 +++++---- cmd/build.go | 2 + cmd/deploy.go | 52 ++++++++++++++++-------- cmd/deploy_test.go | 98 +++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 142 insertions(+), 29 deletions(-) diff --git a/client_int_test.go b/client_int_test.go index e39044fff7..8ed2170d4d 100644 --- a/client_int_test.go +++ b/client_int_test.go @@ -20,16 +20,20 @@ import ( /* NOTE: Running integration tests locally requires a configured test cluster. - Test failures may require manual removal of dangling resources. + Test failures may require manual removal of dangling resources, though + a complete rebuild and destruction of the integration cluster is + suggested. - ## Integration Cluster + ## Creating an Integration-Test Cluster These integration tests require a properly configured cluster, such as that which is setup and configured in CI (see .github/workflows). A local KinD cluster can be started via: ./hack/allocate.sh && ./hack/configure.sh + Note that kubectl, kind and yq are required. on linux they can be installed + with ./hack/binaries.sh - ## Integration Testing + ## Running Integration Tests These tests can be run via the make target: make test-integration @@ -38,10 +42,11 @@ import ( ## Teardown and Cleanup - Tests should clean up after themselves. In the event of failures, one may - need to manually remove files: - rm -rf ./testdata/example.com - The test cluster is not automatically removed, as it can be reused. To remove: + Tests should clean up after themselves, including cluster resources and + temp directoreis. However some tests have not yet been updated, so failed + tests may leave dangling files in ./testdata which must be manually removed. + Additinally, failed tests may not clean up all their cluster resources, so + removing the test cluster after a test run ends in failure is suggested: ./hack/delete.sh */ diff --git a/cmd/build.go b/cmd/build.go index c4f9e2a3f6..ac68378b99 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -151,6 +151,8 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro } f = cfg.Configure(f) // Updates f with build cfg + // TODO: this logic is duplicated with runDeploy. Shouild be in buildConfig + // constructor. // Checks if there is a difference between defined registry and its value used as a prefix in the image tag // In case of a mismatch a new image tag is created and used for build // Do not react if image tag has been changed outside configuration diff --git a/cmd/deploy.go b/cmd/deploy.go index cd620ba33c..de785b577c 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -534,29 +534,34 @@ func newDeployConfig(cmd *cobra.Command) (c deployConfig) { // current values, as they were passed through via flag defaults. func (c deployConfig) Configure(f fn.Function) (fn.Function, error) { var err error + + // Bubble configure request + // // The member values on the config object now take absolute precidence // because they include 1) static config 2) user's global config // 3) Environment variables and 4) flag values (which were set with their // default being 1-3). f = c.buildConfig.Configure(f) // also configures .buildConfig.Global - // deploy adds the ability to specify a digest on the associated image - f.ImageDigest, err = imageDigest(f.Image) - if err != nil { - return f, err - } - // f.Build is not necessary + + // Configure basic members f.Build.Git.URL = c.GitURL f.Build.Git.ContextDir = c.GitDir - f.Build.Git.Revision = c.GitBranch // TODO: shouild match; perhaps be "refSpec" + f.Build.Git.Revision = c.GitBranch // TODO: shouild match; perhaps "refSpec" f.Deploy.Namespace = c.Namespace f.Deploy.Remote = c.Remote - // TODO: validate env test cases completely validate: - envsToUpdate, envsToRemove, err := util.OrderedMapAndRemovalListFromArray(c.Env, "=") + // ImageDigest + // Parsed off f.Image if provided. Deploying adds the ability to specify a + // digest on the associated image (not available on build as nonsensical). + f.ImageDigest, err = imageDigest(f.Image) if err != nil { return f, err } - f.Run.Envs, _, err = mergeEnvs(f.Run.Envs, envsToUpdate, envsToRemove) + + // Envs + // Preprocesses any Envs provided (which may include removals) into a final + // set + f.Run.Envs, err = applyEnvs(f.Run.Envs, c.Env) if err != nil { return f, err } @@ -572,6 +577,20 @@ func (c deployConfig) Configure(f fn.Function) (fn.Function, error) { return f, nil } +// Apply Env additions/removals to a set of extant envs, returning the final +// merged list. +func applyEnvs(current []fn.Env, args []string) (final []fn.Env, err error) { + // TODO: validate env test cases completely validate this functionality + + // Parse and Merge + inserts, removals, err := util.OrderedMapAndRemovalListFromArray(args, "=") + if err != nil { + return + } + final, _, err = mergeEnvs(current, inserts, removals) + return +} + // Prompt the user with value of config members, allowing for interaractive changes. // Skipped if not in an interactive terminal (non-TTY), or if --yes (agree to // all prompts) was explicitly set. @@ -620,7 +639,7 @@ func (c deployConfig) Prompt() (deployConfig, error) { } } - // TODO(lkingland) prompt for optional additional git settings + // TODO: prompt for optional additional git settings here: // if c.GitURL != "" { // } @@ -674,16 +693,15 @@ func (c deployConfig) Validate() (err error) { return fmt.Errorf("invalid --git-url '%v'", c.GitURL) } - // NOTE: (--registry) no explicit check for --registry or --image here, because - // this logic is being baked into the core, and it will validate the cases - // and return fn.ErrNameRequired, fn.ErrImageRequired etc as needed. + // NOTE: There is no explicit check for --registry or --image here, because + // this logic is baked into core, which will validate the cases and return + // an fn.ErrNameRequired, fn.ErrImageRequired etc as needed. return } -// imageDigest splits out and returns the image digest from a full image -// string if it is included, and an error if an image digest is provided but -// is in an invalid format. +// imageDigest returns the image digest from a full image string if it exists, +// and includes basic validation that a provided digest is correctly formatted. func imageDigest(v string) (digest string, err error) { vv := strings.Split(v, "@") if len(vv) < 2 { diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index 9c652f559d..ba9557b950 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "path/filepath" + "reflect" "strings" "testing" "time" @@ -1174,14 +1175,12 @@ func TestDeploy_NamespaceRedeployWarning(t *testing.T) { // TestDeploy_RemotePersists ensures that the remote field is read from // the function by default, and is able to be overridden by flags/env vars. func TestDeploy_RemotePersists(t *testing.T) { - t.Helper() root := fromTempDirectory(t) - cmdFn := NewDeployCmd if err := fn.New().Create(fn.Function{Runtime: "node", Root: root}); err != nil { t.Fatal(err) } - cmd := cmdFn(NewTestClient(fn.WithRegistry(TestRegistry))) + cmd := NewDeployCmd(NewTestClient(fn.WithRegistry(TestRegistry))) cmd.SetArgs([]string{}) if err := cmd.Execute(); err != nil { t.Fatal(err) @@ -1191,7 +1190,7 @@ func TestDeploy_RemotePersists(t *testing.T) { var f fn.Function // Deploy the function, specifying remote deployment(on-cluster) - cmd = cmdFn(NewTestClient(fn.WithRegistry(TestRegistry))) + cmd = NewDeployCmd(NewTestClient(fn.WithRegistry(TestRegistry))) cmd.SetArgs([]string{"--remote"}) if err := cmd.Execute(); err != nil { t.Fatal(err) @@ -1205,7 +1204,7 @@ func TestDeploy_RemotePersists(t *testing.T) { } // Deploy the function again without specifying remote - cmd = cmdFn(NewTestClient(fn.WithRegistry(TestRegistry))) + cmd = NewDeployCmd(NewTestClient(fn.WithRegistry(TestRegistry))) cmd.SetArgs([]string{}) if err := cmd.Execute(); err != nil { t.Fatal(err) @@ -1235,3 +1234,92 @@ func TestDeploy_RemotePersists(t *testing.T) { t.Fatalf("value of remote flag not persisted") } } + +// TestDeploy_Envs ensures that environment variable for the function, provided +// as arguments, are correctly evaluated. This includes: +// - Multiple Envs are supported (flag can be provided multiple times) +// - Existing Envs on the function are retained +// - Flags provided with the minus '-' suffix are treated as a removal +func TestDeploy_Envs(t *testing.T) { + var ( + root = fromTempDirectory(t) + ptr = func(s string) *string { return &s } // TODO: remove pointers from Envs. + f fn.Function + cmd *cobra.Command + err error + expected []fn.Env + ) + + if err = fn.New().Create(fn.Function{Runtime: "go", Root: root, Registry: TestRegistry}); err != nil { + t.Fatal(err) + } + + // Deploy the function with two environment variables specified. + cmd = NewDeployCmd(NewTestClient()) + cmd.SetArgs([]string{"--env=ENV1=VAL1", "--env=ENV2=VAL2"}) + if err = cmd.Execute(); err != nil { + t.Fatal(err) + } + // Assert it contains the two environment variables + if f, err = fn.NewFunction(root); err != nil { + t.Fatal(err) + } + expected = []fn.Env{ + {ptr("ENV1"), ptr("VAL1")}, + {ptr("ENV2"), ptr("VAL2")}, + } + if !reflect.DeepEqual(f.Run.Envs, expected) { + t.Fatalf("Expected envs '%v', got '%v'", expected, f.Run.Envs) + } + + // Deploy the function with an additinal environment variable. + cmd = NewDeployCmd(NewTestClient()) + cmd.SetArgs([]string{"--env=ENV3=VAL3"}) + if err = cmd.Execute(); err != nil { + t.Fatal(err) + } + // Assert the original envs were retained and the new env was added. + if f, err = fn.NewFunction(root); err != nil { + t.Fatal(err) + } + expected = []fn.Env{ + {ptr("ENV1"), ptr("VAL1")}, + {ptr("ENV2"), ptr("VAL2")}, + {ptr("ENV3"), ptr("VAL3")}, + } + if !reflect.DeepEqual(f.Run.Envs, expected) { + t.Fatalf("Expected envs '%v', got '%v'", expected, f.Run.Envs) + } + + // Deploy the function with a removal instruction + cmd = NewDeployCmd(NewTestClient()) + cmd.SetArgs([]string{"--env=ENV1-"}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + if f, err = fn.NewFunction(root); err != nil { + t.Fatal(err) + } + expected = []fn.Env{ + {ptr("ENV2"), ptr("VAL2")}, + {ptr("ENV3"), ptr("VAL3")}, + } + if !reflect.DeepEqual(f.Run.Envs, expected) { + t.Fatalf("Expected envs '%v', got '%v'", expected, f.Run.Envs) + } + + // Try removing the rest for good measure + cmd = NewDeployCmd(NewTestClient()) + cmd.SetArgs([]string{"--env=ENV2-", "--env=ENV3-"}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + if f, err = fn.NewFunction(root); err != nil { + t.Fatal(err) + } + if len(f.Run.Envs) != 0 { + t.Fatalf("Expected no envs to remain, got '%v'", f.Run.Envs) + } + + // TODO: create and test typed errors for ErrEnvNotExist etc. +}