From fdcd9be3c96a686d530859b4f777ea5b45cf5eda Mon Sep 17 00:00:00 2001 From: Dan Duvall Date: Sat, 19 May 2018 20:12:34 +0200 Subject: 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 --- README.md | 2 +- blubber.example.yaml | 2 +- config/flag_test.go | 18 +++++++++--------- config/node.go | 30 ++++++++++++++++-------------- config/node_test.go | 30 ++++++++++++++++-------------- config/policy_test.go | 8 ++++---- config/reader_test.go | 30 ++++++++++++++++-------------- 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) { -- cgit v1.2.1