From 0397929d3482449f3a73044d3cef6630a6a5d8b5 Mon Sep 17 00:00:00 2001 From: Stephen Cuppett Date: Fri, 11 Aug 2023 09:31:36 -0400 Subject: [PATCH 1/4] Resolves #1124: Always scan the repository for provided scripts Whether the base image provides the scripts or not, the repository can supply override scripts. Removes the expectation/need of a location specified in the image. --- pkg/build/strategies/dockerfile/dockerfile.go | 17 +++++++---------- .../strategies/dockerfile/dockerfile_test.go | 5 +---- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/pkg/build/strategies/dockerfile/dockerfile.go b/pkg/build/strategies/dockerfile/dockerfile.go index d8e66009b..69309bff3 100644 --- a/pkg/build/strategies/dockerfile/dockerfile.go +++ b/pkg/build/strategies/dockerfile/dockerfile.go @@ -127,11 +127,8 @@ 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 := getImageScriptsDir(config) + providedScripts := scanScripts(filepath.Join(config.WorkingDir, builder.uploadScriptsDir)) if config.Incremental { imageTag := util.FirstNonEmpty(config.IncrementalFromTag, config.Tag) @@ -424,18 +421,18 @@ func getDestination(config *api.Config) string { // 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) { +func getImageScriptsDir(config *api.Config) string { if strings.HasPrefix(config.ScriptsURL, "image://") { - return strings.TrimPrefix(config.ScriptsURL, "image://"), true + return strings.TrimPrefix(config.ScriptsURL, "image://") } scriptsURL, _ := util.AdjustConfigWithImageLabels(config) if strings.HasPrefix(scriptsURL, "image://") { - return strings.TrimPrefix(scriptsURL, "image://"), true + return strings.TrimPrefix(scriptsURL, "image://") } if strings.HasPrefix(config.ImageScriptsURL, "image://") { - return strings.TrimPrefix(config.ImageScriptsURL, "image://"), false + return strings.TrimPrefix(config.ImageScriptsURL, "image://") } - return defaultScriptsDir, false + return defaultScriptsDir } // scanScripts returns a map of provided s2i scripts diff --git a/pkg/build/strategies/dockerfile/dockerfile_test.go b/pkg/build/strategies/dockerfile/dockerfile_test.go index a56312e34..222b14615 100644 --- a/pkg/build/strategies/dockerfile/dockerfile_test.go +++ b/pkg/build/strategies/dockerfile/dockerfile_test.go @@ -94,13 +94,10 @@ func TestGetImageScriptsDir(t *testing.T) { }, } for _, tc := range cases { - output, hasScripts := getImageScriptsDir(tc.Config) + output := getImageScriptsDir(tc.Config) 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) - } } } From 83e9e15c1c928626469c5f545219c2035f2b59c7 Mon Sep 17 00:00:00 2001 From: Stephen Cuppett Date: Thu, 26 Oct 2023 14:14:20 -0400 Subject: [PATCH 2/4] Revert "Resolves #1124: Always scan the repository for provided scripts" This reverts commit 0397929d3482449f3a73044d3cef6630a6a5d8b5. --- pkg/build/strategies/dockerfile/dockerfile.go | 17 ++++++++++------- .../strategies/dockerfile/dockerfile_test.go | 5 ++++- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/pkg/build/strategies/dockerfile/dockerfile.go b/pkg/build/strategies/dockerfile/dockerfile.go index 69309bff3..d8e66009b 100644 --- a/pkg/build/strategies/dockerfile/dockerfile.go +++ b/pkg/build/strategies/dockerfile/dockerfile.go @@ -127,8 +127,11 @@ 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 := getImageScriptsDir(config) - providedScripts := scanScripts(filepath.Join(config.WorkingDir, builder.uploadScriptsDir)) + imageScriptsDir, hasAllScripts := getImageScriptsDir(config) + var providedScripts map[string]bool + if !hasAllScripts { + providedScripts = scanScripts(filepath.Join(config.WorkingDir, builder.uploadScriptsDir)) + } if config.Incremental { imageTag := util.FirstNonEmpty(config.IncrementalFromTag, config.Tag) @@ -421,18 +424,18 @@ func getDestination(config *api.Config) string { // 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 { +func getImageScriptsDir(config *api.Config) (string, bool) { if strings.HasPrefix(config.ScriptsURL, "image://") { - return strings.TrimPrefix(config.ScriptsURL, "image://") + return strings.TrimPrefix(config.ScriptsURL, "image://"), true } scriptsURL, _ := util.AdjustConfigWithImageLabels(config) if strings.HasPrefix(scriptsURL, "image://") { - return strings.TrimPrefix(scriptsURL, "image://") + return strings.TrimPrefix(scriptsURL, "image://"), true } if strings.HasPrefix(config.ImageScriptsURL, "image://") { - return strings.TrimPrefix(config.ImageScriptsURL, "image://") + return strings.TrimPrefix(config.ImageScriptsURL, "image://"), false } - return defaultScriptsDir + return defaultScriptsDir, false } // scanScripts returns a map of provided s2i scripts diff --git a/pkg/build/strategies/dockerfile/dockerfile_test.go b/pkg/build/strategies/dockerfile/dockerfile_test.go index 222b14615..a56312e34 100644 --- a/pkg/build/strategies/dockerfile/dockerfile_test.go +++ b/pkg/build/strategies/dockerfile/dockerfile_test.go @@ -94,10 +94,13 @@ func TestGetImageScriptsDir(t *testing.T) { }, } for _, tc := range cases { - output := getImageScriptsDir(tc.Config) + output, hasScripts := getImageScriptsDir(tc.Config) 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) + } } } From f849cd7789be03c0cc0e22725c1a86a37060edd1 Mon Sep 17 00:00:00 2001 From: Stephen Cuppett Date: Thu, 26 Oct 2023 15:30:52 -0400 Subject: [PATCH 3/4] Resolves #1124: Always scan the repository for provided scripts Whether the base image provides the scripts or not, the repository can supply override scripts. Removes the expectation/need of a location specified in the image. --- pkg/build/strategies/dockerfile/dockerfile.go | 25 +++++++++++-------- .../strategies/dockerfile/dockerfile_test.go | 20 +++++---------- 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/pkg/build/strategies/dockerfile/dockerfile.go b/pkg/build/strategies/dockerfile/dockerfile.go index d8e66009b..44f359cd8 100644 --- a/pkg/build/strategies/dockerfile/dockerfile.go +++ b/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) @@ -424,18 +420,27 @@ func getDestination(config *api.Config) string { // 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) { +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 diff --git a/pkg/build/strategies/dockerfile/dockerfile_test.go b/pkg/build/strategies/dockerfile/dockerfile_test.go index a56312e34..e9801ad8e 100644 --- a/pkg/build/strategies/dockerfile/dockerfile_test.go +++ b/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,8 +60,7 @@ 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{ @@ -70,8 +68,7 @@ func TestGetImageScriptsDir(t *testing.T) { constants.ScriptsURLLabel: "image:///usr/some/dir", }, }, - ExpectedDir: "/usr/some/dir", - HasAllScripts: true, + ExpectedDir: "/usr/some/dir", }, { Config: &api.Config{ @@ -80,8 +77,7 @@ func TestGetImageScriptsDir(t *testing.T) { constants.DeprecatedScriptsURLLabel: "image:///usr/other/dir", }, }, - ExpectedDir: "/usr/some/dir", - HasAllScripts: true, + ExpectedDir: "/usr/some/dir", }, { Config: &api.Config{ @@ -89,18 +85,14 @@ func TestGetImageScriptsDir(t *testing.T) { 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) - } } } From 63fa22d8bdda056f082c7d05a396dcfc6f4c1e58 Mon Sep 17 00:00:00 2001 From: Stephen Cuppett Date: Thu, 2 Nov 2023 13:52:29 -0400 Subject: [PATCH 4/4] Nit: Comment update to getImageScriptsDir --- pkg/build/strategies/dockerfile/dockerfile.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/build/strategies/dockerfile/dockerfile.go b/pkg/build/strategies/dockerfile/dockerfile.go index 44f359cd8..a25ac42ab 100644 --- a/pkg/build/strategies/dockerfile/dockerfile.go +++ b/pkg/build/strategies/dockerfile/dockerfile.go @@ -418,8 +418,8 @@ 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 +// 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)