Merge pull request #1125 from cuppett/cuppett/fix-1124

Resolves #1124: Always scan the repository for provided scripts

pkg/build/strategies/dockerfile/dockerfile.go

@@ -127,11 +127,7 @@ func (builder *Dockerfile) CreateDockerfile(config *api.Config) error {
 	artifactsDestDir := filepath.Join(getDestination(config), "artifacts")
 	artifactsTar := sanitize(filepath.ToSlash(filepath.Join(defaultDestination, "artifacts.tar")))
 	// hasAllScripts indicates that we blindly trust all scripts are provided in the image scripts dir
-	imageScriptsDir, hasAllScripts := getImageScriptsDir(config)
-	var providedScripts map[string]bool
-	if !hasAllScripts {
-		providedScripts = scanScripts(filepath.Join(config.WorkingDir, builder.uploadScriptsDir))
-	}
+	imageScriptsDir, providedScripts := getImageScriptsDir(config, builder)
 
 	if config.Incremental {
 		imageTag := util.FirstNonEmpty(config.IncrementalFromTag, config.Tag)
@@ -422,20 +418,29 @@ func getDestination(config *api.Config) string {
 	return destination
 }
 
-// getImageScriptsDir returns the directory containing the builder image scripts and a bool
-// indicating that the directory is expected to contain all s2i scripts
-func getImageScriptsDir(config *api.Config) (string, bool) {
+// getImageScriptsDir returns the default directory which should contain the builder image scripts
+// as well as a map of booleans identifying  individual scripts provided in the repository as overrides
+func getImageScriptsDir(config *api.Config, builder *Dockerfile) (string, map[string]bool) {
+
+	// 1st priority is the command line parameter (pointing to an image, overrides it all)
 	if strings.HasPrefix(config.ScriptsURL, "image://") {
-		return strings.TrimPrefix(config.ScriptsURL, "image://"), true
+		return strings.TrimPrefix(config.ScriptsURL, "image://"), make(map[string]bool)
 	}
+
+	// 2nd priority (the source code repository), collect the locations
+	providedScripts := scanScripts(filepath.Join(config.WorkingDir, builder.uploadScriptsDir))
+
+	// 3rd priority (the builder image), collect the locations
 	scriptsURL, _ := util.AdjustConfigWithImageLabels(config)
 	if strings.HasPrefix(scriptsURL, "image://") {
-		return strings.TrimPrefix(scriptsURL, "image://"), true
+		return strings.TrimPrefix(scriptsURL, "image://"), providedScripts
 	}
 	if strings.HasPrefix(config.ImageScriptsURL, "image://") {
-		return strings.TrimPrefix(config.ImageScriptsURL, "image://"), false
+		return strings.TrimPrefix(config.ImageScriptsURL, "image://"), providedScripts
 	}
-	return defaultScriptsDir, false
+
+	// If all else fails, use the default scripts dir
+	return defaultScriptsDir, providedScripts
 }
 
 // scanScripts returns a map of provided s2i scripts

pkg/build/strategies/dockerfile/dockerfile_test.go

@@ -28,8 +28,7 @@ func TestGetImageScriptsDir(t *testing.T) {
 			Config: &api.Config{
 				ScriptsURL: "image:///usr/some/dir",
 			},
-			ExpectedDir:   "/usr/some/dir",
-			HasAllScripts: true,
+			ExpectedDir: "/usr/some/dir",
 		},
 		{
 			Config: &api.Config{
@@ -61,17 +60,15 @@ func TestGetImageScriptsDir(t *testing.T) {
 				ScriptsURL:      "image:///usr/some/dir",
 				ImageScriptsURL: "image:///usr/other/dir",
 			},
-			ExpectedDir:   "/usr/some/dir",
-			HasAllScripts: true,
+			ExpectedDir: "/usr/some/dir",
 		},
 		{
 			Config: &api.Config{
 				BuilderImageLabels: map[string]string{
 					constants.ScriptsURLLabel: "image:///usr/some/dir",
 				},
 			},
-			ExpectedDir:   "/usr/some/dir",
-			HasAllScripts: true,
+			ExpectedDir: "/usr/some/dir",
 		},
 		{
 			Config: &api.Config{
@@ -80,27 +77,22 @@ func TestGetImageScriptsDir(t *testing.T) {
 					constants.DeprecatedScriptsURLLabel: "image:///usr/other/dir",
 				},
 			},
-			ExpectedDir:   "/usr/some/dir",
-			HasAllScripts: true,
+			ExpectedDir: "/usr/some/dir",
 		},
 		{
 			Config: &api.Config{
 				BuilderImageLabels: map[string]string{
 					constants.DeprecatedScriptsURLLabel: "image:///usr/some/dir",
 				},
 			},
-			ExpectedDir:   "/usr/some/dir",
-			HasAllScripts: true,
+			ExpectedDir: "/usr/some/dir",
 		},
 	}
 	for _, tc := range cases {
-		output, hasScripts := getImageScriptsDir(tc.Config)
+		output, _ := getImageScriptsDir(tc.Config, &(Dockerfile{}))
 		if output != tc.ExpectedDir {
 			t.Errorf("Expected image scripts dir %s to be %s", output, tc.ExpectedDir)
 		}
-		if hasScripts != tc.HasAllScripts {
-			t.Errorf("Expected has all scripts indicator:\n%v\nto be: %v", hasScripts, tc.HasAllScripts)
-		}
 	}
 }