Refactor config for node app dependencies

Summary:
Change `node.dependencies` flag to `node.requirements`, the list of
files (typically `package.json` and either `npm-shrinkwrap.json` or
`package-lock.json`) that declare dependencies to be installed with NPM.

Test Plan: Run `go test ./...`. Try it out with something like Mathoid.

Reviewers: thcipriani, hashar, mobrovac, Joe, akosiaris, #release-engineering-team

Reviewed By: thcipriani, #release-engineering-team

Tags: #release-engineering-team

Differential Revision: https://phabricator.wikimedia.org/D1058

7 files changed, 63 insertions, 57 deletions

diff --git a/README.md b/README.md
index 2183154..f8ee4a1 100644
--- a/README.md
+++ b/README.md
@@ -28,7 +28,7 @@ variants:
     apt:
       packages: [libjpeg-dev, libyaml-dev]
     node:
-      dependencies: true
+      requirements: [package.json, package-lock.json]
 
   development:
     includes: [build]
diff --git a/blubber.example.yaml b/blubber.example.yaml
index 80eddc3..25df24e 100644
--- a/blubber.example.yaml
+++ b/blubber.example.yaml
@@ -15,7 +15,7 @@ variants:
     apt:
       packages: [libjpeg-dev, libyaml-dev]
     node:
-      dependencies: true
+      requirements: [package.json, package-lock.json]
     python:
       requirements: [requirements.txt]
     builder: [make, -f, Makefile]
diff --git a/config/flag_test.go b/config/flag_test.go
index ef6fbf4..e637faf 100644
--- a/config/flag_test.go
+++ b/config/flag_test.go
@@ -12,19 +12,19 @@ func TestFlagMerge(t *testing.T) {
 	cfg, err := config.ReadConfig([]byte(`---
     version: v1
     base: foo
-    node: { dependencies: true }
+    runs: { insecurely: true }
     sharedvolume: false
     variants:
       development:
         sharedvolume: true
-        node: { dependencies: false }`))
+        runs: { insecurely: false }`))
 
-	assert.Nil(t, err)
+	if assert.NoError(t, err) {
+		variant, err := config.ExpandVariant(cfg, "development")
 
-	variant, err := config.ExpandVariant(cfg, "development")
-
-	assert.Nil(t, err)
-
-	assert.False(t, variant.Node.Dependencies.True)
-	assert.True(t, variant.SharedVolume.True)
+		if assert.NoError(t, err) {
+			assert.False(t, variant.Runs.Insecurely.True)
+			assert.True(t, variant.SharedVolume.True)
+		}
+	}
 }
diff --git a/config/node.go b/config/node.go
index bd5d4db..5834443 100644
--- a/config/node.go
+++ b/config/node.go
@@ -9,15 +9,17 @@ import (
 // whether/how to install NPM packages.
 //
 type NodeConfig struct {
-	Dependencies Flag   `yaml:"dependencies"`                     // install dependencies declared in package.json
-	Env          string `yaml:"env" validate:"omitempty,nodeenv"` // environment name ("production" install)
+	Requirements []string `yaml:"requirements"`                     // install requirements from given files
+	Env          string   `yaml:"env" validate:"omitempty,nodeenv"` // environment name ("production" install)
 }
 
 // Merge takes another NodeConfig and merges its fields into this one's,
-// overwriting both the environment and dependencies flag.
+// overwriting both the environment and requirements files.
 //
 func (nc *NodeConfig) Merge(nc2 NodeConfig) {
-	nc.Dependencies.Merge(nc2.Dependencies)
+	if nc2.Requirements != nil {
+		nc.Requirements = nc2.Requirements
+	}
 
 	if nc2.Env != "" {
 		nc.Env = nc2.Env
@@ -30,13 +32,13 @@ func (nc *NodeConfig) Merge(nc2 NodeConfig) {
 //
 // PhasePreInstall
 //
-// Installs Node package dependencies declared in package.json into the shared
-// library directory (/opt/lib). Only production related packages are install
-// if NodeConfig.Env is set to "production" in which case `npm dedupe` is also
-// run. Installing dependencies during the build.PhasePreInstall phase allows
-// a compiler implementation (e.g. Docker) to produce cache-efficient output
-// so only changes to package.json will invalidate these steps of the image
-// build.
+// Installs Node package dependencies declared in requirements files into the
+// shared library directory (/opt/lib). Only production related packages are
+// install if NodeConfig.Env is set to "production" in which case `npm dedupe`
+// is also run. Installing dependencies during the build.PhasePreInstall phase
+// allows a compiler implementation (e.g. Docker) to produce cache-efficient
+// output so only changes to package.json will invalidate these steps of the
+// image build.
 //
 // PhasePostInstall
 //
@@ -48,7 +50,7 @@ func (nc *NodeConfig) Merge(nc2 NodeConfig) {
 func (nc NodeConfig) InstructionsForPhase(phase build.Phase) []build.Instruction {
 	switch phase {
 	case build.PhasePreInstall:
-		if nc.Dependencies.True {
+		if len(nc.Requirements) > 0 {
 			npmInstall := build.RunAll{[]build.Run{
 				{"cd", []string{LocalLibPrefix}},
 				{"npm install", []string{}},
@@ -62,12 +64,12 @@ func (nc NodeConfig) InstructionsForPhase(phase build.Phase) []build.Instruction
 			}
 
 			return []build.Instruction{
-				build.Copy{[]string{"package.json"}, LocalLibPrefix},
+				build.Copy{nc.Requirements, LocalLibPrefix},
 				npmInstall,
 			}
 		}
 	case build.PhasePostInstall:
-		if nc.Env != "" || nc.Dependencies.True {
+		if nc.Env != "" || len(nc.Requirements) > 0 {
 			return []build.Instruction{build.Env{map[string]string{
 				"NODE_ENV":  nc.Env,
 				"NODE_PATH": path.Join(LocalLibPrefix, "node_modules"),
diff --git a/config/node_test.go b/config/node_test.go
index 5d5a39e..f39b6cf 100644
--- a/config/node_test.go
+++ b/config/node_test.go
@@ -14,27 +14,29 @@ func TestNodeConfigYAML(t *testing.T) {
     version: v1
     base: foo
     node:
-      dependencies: true
+      requirements: [package.json]
       env: foo
     variants:
       build:
         node:
-          dependencies: false
+          requirements: []
           env: bar`))
 
-	assert.Nil(t, err)
+	if assert.NoError(t, err) {
+		assert.Equal(t, []string{"package.json"}, cfg.Node.Requirements)
+		assert.Equal(t, "foo", cfg.Node.Env)
 
-	assert.Equal(t, true, cfg.Node.Dependencies.True)
-	assert.Equal(t, "foo", cfg.Node.Env)
+		variant, err := config.ExpandVariant(cfg, "build")
 
-	variant, err := config.ExpandVariant(cfg, "build")
-
-	assert.Equal(t, false, variant.Node.Dependencies.True)
-	assert.Equal(t, "bar", variant.Node.Env)
+		if assert.NoError(t, err) {
+			assert.Empty(t, variant.Node.Requirements)
+			assert.Equal(t, "bar", variant.Node.Env)
+		}
+	}
 }
 
-func TestNodeConfigInstructionsNoDependencies(t *testing.T) {
-	cfg := config.NodeConfig{Dependencies: config.Flag{True: false}}
+func TestNodeConfigInstructionsNoRequirements(t *testing.T) {
+	cfg := config.NodeConfig{}
 
 	t.Run("PhasePrivileged", func(t *testing.T) {
 		assert.Empty(t, cfg.InstructionsForPhase(build.PhasePrivileged))
@@ -54,7 +56,7 @@ func TestNodeConfigInstructionsNoDependencies(t *testing.T) {
 }
 
 func TestNodeConfigInstructionsNonProduction(t *testing.T) {
-	cfg := config.NodeConfig{Dependencies: config.Flag{True: true}, Env: "foo"}
+	cfg := config.NodeConfig{Requirements: []string{"package.json"}, Env: "foo"}
 
 	t.Run("PhasePrivileged", func(t *testing.T) {
 		assert.Empty(t, cfg.InstructionsForPhase(build.PhasePrivileged))
@@ -92,7 +94,7 @@ func TestNodeConfigInstructionsNonProduction(t *testing.T) {
 }
 
 func TestNodeConfigInstructionsProduction(t *testing.T) {
-	cfg := config.NodeConfig{Dependencies: config.Flag{True: true}, Env: "production"}
+	cfg := config.NodeConfig{Requirements: []string{"package.json", "package-lock.json"}, Env: "production"}
 
 	t.Run("PhasePrivileged", func(t *testing.T) {
 		assert.Empty(t, cfg.InstructionsForPhase(build.PhasePrivileged))
@@ -105,7 +107,7 @@ func TestNodeConfigInstructionsProduction(t *testing.T) {
 	t.Run("PhasePreInstall", func(t *testing.T) {
 		assert.Equal(t,
 			[]build.Instruction{
-				build.Copy{[]string{"package.json"}, "/opt/lib"},
+				build.Copy{[]string{"package.json", "package-lock.json"}, "/opt/lib"},
 				build.RunAll{[]build.Run{
 					{"cd", []string{"/opt/lib"}},
 					{"npm install", []string{"--production"}},
diff --git a/config/policy_test.go b/config/policy_test.go
index 2d7a3a7..80de387 100644
--- a/config/policy_test.go
+++ b/config/policy_test.go
@@ -73,8 +73,8 @@ func TestEnforcementOnFlag(t *testing.T) {
 		Variants: map[string]config.VariantConfig{
 			"production": config.VariantConfig{
 				CommonConfig: config.CommonConfig{
-					Node: config.NodeConfig{
-						Dependencies: config.Flag{True: true},
+					Runs: config.RunsConfig{
+						Insecurely: config.Flag{True: true},
 					},
 				},
 			},
@@ -83,13 +83,13 @@ func TestEnforcementOnFlag(t *testing.T) {
 
 	policy := config.Policy{
 		Enforcements: []config.Enforcement{
-			{Path: "variants.production.node.dependencies", Rule: "isfalse"},
+			{Path: "variants.production.runs.insecurely", Rule: "isfalse"},
 		},
 	}
 
 	assert.Error(t,
 		policy.Validate(cfg),
-		`value for "variants.production.node.dependencies" violates policy rule "isfalse"`,
+		`value for "variants.production.runs.insecurely" violates policy rule "isfalse"`,
 	)
 
 }
diff --git a/config/reader_test.go b/config/reader_test.go
index 933e388..8943adc 100644
--- a/config/reader_test.go
+++ b/config/reader_test.go
@@ -72,31 +72,33 @@ func TestResolveIncludesPreventsInfiniteRecursion(t *testing.T) {
 func TestMultiLevelIncludes(t *testing.T) {
 	cfg, err := config.ReadConfig([]byte(`---
     version: v1
-    base: nodejs-slim
+    base: foo-slim
     variants:
       build:
-        base: nodejs-devel
-        node: {env: build}
+        base: foo-devel
+        runs: { as: foo }
       development:
         includes: [build]
-        node: {env: development}
-        entrypoint: [npm, start]
+        runs: { uid: 123 }
       test:
         includes: [development]
-        node: {dependencies: true}
-        entrypoint: [npm, test]`))
+        runs: { insecurely: true }`))
 
-	assert.NoError(t, err)
+	if assert.NoError(t, err) {
+		dev, _ := config.ExpandVariant(cfg, "development")
 
-	variant, _ := config.ExpandVariant(cfg, "test")
+		assert.Equal(t, "foo-devel", dev.Base)
+		assert.Equal(t, "foo", dev.Runs.As)
+		assert.Equal(t, uint(123), dev.Runs.UID)
 
-	assert.Equal(t, "nodejs-devel", variant.Base)
-	assert.Equal(t, "development", variant.Node.Env)
+		test, _ := config.ExpandVariant(cfg, "test")
 
-	devVariant, _ := config.ExpandVariant(cfg, "development")
+		assert.Equal(t, "foo-devel", test.Base)
+		assert.Equal(t, "foo", test.Runs.As)
+		assert.Equal(t, uint(123), test.Runs.UID)
 
-	assert.True(t, variant.Node.Dependencies.True)
-	assert.False(t, devVariant.Node.Dependencies.True)
+		assert.True(t, test.Runs.Insecurely.True)
+	}
 }
 
 func TestMultiIncludes(t *testing.T) {