From 1804a50fab3063210683419e8efd359ef1d3696b Mon Sep 17 00:00:00 2001 From: Paulo Bittencourt Date: Mon, 19 Aug 2024 21:46:23 -0400 Subject: [PATCH 1/4] feat: add remote taskfile HTTP tests --- task_test.go | 104 ++++++++++++++++++ testdata/includes_remote/.gitignore | 1 + testdata/includes_remote/Taskfile.yml | 4 + testdata/includes_remote/first/Taskfile.yml | 11 ++ .../includes_remote/first/second/Taskfile.yml | 8 ++ 5 files changed, 128 insertions(+) create mode 100644 testdata/includes_remote/.gitignore create mode 100644 testdata/includes_remote/Taskfile.yml create mode 100644 testdata/includes_remote/first/Taskfile.yml create mode 100644 testdata/includes_remote/first/second/Taskfile.yml diff --git a/task_test.go b/task_test.go index 3411c5ebd0..e90d8074d6 100644 --- a/task_test.go +++ b/task_test.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "io/fs" + "math/rand" "net/http" "net/http/httptest" "os" @@ -29,6 +30,8 @@ import ( "github.com/go-task/task/v3/taskfile/ast" ) +var random = rand.New(rand.NewSource(time.Now().UnixNano())) + func init() { _ = os.Setenv("NO_COLOR", "1") } @@ -1047,6 +1050,107 @@ func TestIncludesMultiLevel(t *testing.T) { tt.Run(t) } +func TestIncludesRemote(t *testing.T) { + enableExperimentForTest(t, &experiments.RemoteTaskfiles, "1") + + dir := "testdata/includes_remote" + + srv := httptest.NewServer(http.FileServer(http.Dir(dir))) + defer srv.Close() + + tcs := []struct { + firstRemote string + secondRemote string + }{ + { + firstRemote: srv.URL + "/first/Taskfile.yml", + secondRemote: srv.URL + "/first/second/Taskfile.yml", + }, + { + firstRemote: srv.URL + "/first/Taskfile.yml", + secondRemote: "./second/Taskfile.yml", + }, + } + + tasks := []string{ + "first:write-file", + "first:second:write-file", + } + + for i, tc := range tcs { + t.Run(fmt.Sprint(i), func(t *testing.T) { + t.Setenv("FIRST_REMOTE_URL", tc.firstRemote) + t.Setenv("SECOND_REMOTE_URL", tc.secondRemote) + + var buff SyncBuffer + + executors := []struct { + name string + executor *task.Executor + }{ + { + name: "online, always download", + executor: &task.Executor{ + Dir: dir, + Stdout: &buff, + Stderr: &buff, + Timeout: time.Minute, + Insecure: true, + Logger: &logger.Logger{Stdout: &buff, Stderr: &buff, Verbose: true}, + + // Without caching + AssumeYes: true, + Download: true, + }, + }, + { + name: "offline, use cache", + executor: &task.Executor{ + Dir: dir, + Stdout: &buff, + Stderr: &buff, + Timeout: time.Minute, + Insecure: true, + Logger: &logger.Logger{Stdout: &buff, Stderr: &buff, Verbose: true}, + + // With caching + AssumeYes: false, + Download: false, + Offline: true, + }, + }, + } + + for j, e := range executors { + t.Run(fmt.Sprint(j), func(t *testing.T) { + require.NoError(t, e.executor.Setup()) + + for k, task := range tasks { + t.Run(task, func(t *testing.T) { + expectedContent := fmt.Sprint(random.Int63()) + t.Setenv("CONTENT", expectedContent) + + outputFile := fmt.Sprintf("%d.%d.txt", i, k) + t.Setenv("OUTPUT_FILE", outputFile) + + path := filepath.Join(dir, outputFile) + require.NoError(t, os.RemoveAll(path)) + + require.NoError(t, e.executor.Run(context.Background(), &ast.Call{Task: task})) + + actualContent, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, expectedContent, strings.TrimSpace(string(actualContent))) + }) + } + }) + } + + t.Log("\noutput:\n", buff.buf.String()) + }) + } +} + func TestIncludeCycle(t *testing.T) { const dir = "testdata/includes_cycle" diff --git a/testdata/includes_remote/.gitignore b/testdata/includes_remote/.gitignore new file mode 100644 index 0000000000..2211df63dd --- /dev/null +++ b/testdata/includes_remote/.gitignore @@ -0,0 +1 @@ +*.txt diff --git a/testdata/includes_remote/Taskfile.yml b/testdata/includes_remote/Taskfile.yml new file mode 100644 index 0000000000..b518102245 --- /dev/null +++ b/testdata/includes_remote/Taskfile.yml @@ -0,0 +1,4 @@ +version: '3' + +includes: + first: "{{.FIRST_REMOTE_URL}}" diff --git a/testdata/includes_remote/first/Taskfile.yml b/testdata/includes_remote/first/Taskfile.yml new file mode 100644 index 0000000000..d15e50e0ec --- /dev/null +++ b/testdata/includes_remote/first/Taskfile.yml @@ -0,0 +1,11 @@ +version: '3' + +includes: + second: "{{.SECOND_REMOTE_URL}}" + +tasks: + write-file: + requires: + vars: [CONTENT, OUTPUT_FILE] + cmd: | + echo "{{.CONTENT}}" > "{{.OUTPUT_FILE}}" diff --git a/testdata/includes_remote/first/second/Taskfile.yml b/testdata/includes_remote/first/second/Taskfile.yml new file mode 100644 index 0000000000..1b261e448d --- /dev/null +++ b/testdata/includes_remote/first/second/Taskfile.yml @@ -0,0 +1,8 @@ +version: '3' + +tasks: + write-file: + requires: + vars: [CONTENT, OUTPUT_FILE] + cmd: | + echo "{{.CONTENT}}" > "{{.OUTPUT_FILE}}" From f704a0629e546b1001c9c2a1eef624e3d4db4476 Mon Sep 17 00:00:00 2001 From: Paulo Bittencourt Date: Wed, 21 Aug 2024 16:22:59 -0400 Subject: [PATCH 2/4] refactor: extract node content loading to separate method --- taskfile/reader.go | 67 ++++++++++++++++++++++++++-------------------- 1 file changed, 38 insertions(+), 29 deletions(-) diff --git a/taskfile/reader.go b/taskfile/reader.go index c64f35b1a9..f3e41392a9 100644 --- a/taskfile/reader.go +++ b/taskfile/reader.go @@ -184,6 +184,43 @@ func (r *Reader) include(node Node) error { } func (r *Reader) readNode(node Node) (*ast.Taskfile, error) { + b, err := r.loadNodeContent(node) + if err != nil { + return nil, err + } + + var tf ast.Taskfile + if err := yaml.Unmarshal(b, &tf); err != nil { + // Decode the taskfile and add the file info the any errors + taskfileInvalidErr := &errors.TaskfileDecodeError{} + if errors.As(err, &taskfileInvalidErr) { + return nil, taskfileInvalidErr.WithFileInfo(node.Location(), b, 2) + } + return nil, &errors.TaskfileInvalidError{URI: filepathext.TryAbsToRel(node.Location()), Err: err} + } + + // Check that the Taskfile is set and has a schema version + if tf.Version == nil { + return nil, &errors.TaskfileVersionCheckError{URI: node.Location()} + } + + // Set the taskfile/task's locations + tf.Location = node.Location() + for _, task := range tf.Tasks.Values() { + // If the task is not defined, create a new one + if task == nil { + task = &ast.Task{} + } + // Set the location of the taskfile for each task + if task.Location.Taskfile == "" { + task.Location.Taskfile = tf.Location + } + } + + return &tf, nil +} + +func (r *Reader) loadNodeContent(node Node) ([]byte, error) { var b []byte var err error var cache *Cache @@ -272,33 +309,5 @@ func (r *Reader) readNode(node Node) (*ast.Taskfile, error) { } } - var tf ast.Taskfile - if err := yaml.Unmarshal(b, &tf); err != nil { - // Decode the taskfile and add the file info the any errors - taskfileInvalidErr := &errors.TaskfileDecodeError{} - if errors.As(err, &taskfileInvalidErr) { - return nil, taskfileInvalidErr.WithFileInfo(node.Location(), b, 2) - } - return nil, &errors.TaskfileInvalidError{URI: filepathext.TryAbsToRel(node.Location()), Err: err} - } - - // Check that the Taskfile is set and has a schema version - if tf.Version == nil { - return nil, &errors.TaskfileVersionCheckError{URI: node.Location()} - } - - // Set the taskfile/task's locations - tf.Location = node.Location() - for _, task := range tf.Tasks.Values() { - // If the task is not defined, create a new one - if task == nil { - task = &ast.Task{} - } - // Set the location of the taskfile for each task - if task.Location.Taskfile == "" { - task.Location.Taskfile = tf.Location - } - } - - return &tf, nil + return b, nil } From ab18f5ec37dae4443d4924297d0b62dd38d38fe7 Mon Sep 17 00:00:00 2001 From: Paulo Bittencourt Date: Wed, 21 Aug 2024 16:51:18 -0400 Subject: [PATCH 3/4] refactor: re-organize node loading code to make it easier to follow Readability is subjective, but while working in this part of the code I found it difficult to follow the different code paths when loading a node's content. In this change I am attempting to make the branches of the decision tree return early as much as possible. This way we discard each possibility as we work our way down, in this order: 1. Non-remote nodes 2. In offline mode, use cached remote nodes 3. On network timeout, use cached remote nodes 4a. Use fetched remote node 4b. On checksum change, handle writing to cache Tests are added to ensure this functionality does not break. --- taskfile/reader.go | 136 ++++++++++++++++++++++----------------------- 1 file changed, 67 insertions(+), 69 deletions(-) diff --git a/taskfile/reader.go b/taskfile/reader.go index f3e41392a9..69f75b1d19 100644 --- a/taskfile/reader.go +++ b/taskfile/reader.go @@ -221,91 +221,89 @@ func (r *Reader) readNode(node Node) (*ast.Taskfile, error) { } func (r *Reader) loadNodeContent(node Node) ([]byte, error) { - var b []byte - var err error - var cache *Cache + if !node.Remote() { + ctx, cf := context.WithTimeout(context.Background(), r.timeout) + defer cf() + return node.Read(ctx) + } - if node.Remote() { - cache, err = NewCache(r.tempDir) - if err != nil { - return nil, err - } + cache, err := NewCache(r.tempDir) + if err != nil { + return nil, err } - // If the file is remote and we're in offline mode, check if we have a cached copy - if node.Remote() && r.offline { - if b, err = cache.read(node); errors.Is(err, os.ErrNotExist) { + if r.offline { + // In offline mode try to use cached copy + cached, err := cache.read(node) + if errors.Is(err, os.ErrNotExist) { return nil, &errors.TaskfileCacheNotFoundError{URI: node.Location()} } else if err != nil { return nil, err } r.logger.VerboseOutf(logger.Magenta, "task: [%s] Fetched cached copy\n", node.Location()) - } else { - downloaded := false - ctx, cf := context.WithTimeout(context.Background(), r.timeout) - defer cf() + return cached, nil + } - // Read the file - b, err = node.Read(ctx) - var taskfileNetworkTimeoutError *errors.TaskfileNetworkTimeoutError + ctx, cf := context.WithTimeout(context.Background(), r.timeout) + defer cf() + + b, err := node.Read(ctx) + if errors.Is(err, &errors.TaskfileNetworkTimeoutError{}) { // If we timed out then we likely have a network issue - if node.Remote() && errors.As(err, &taskfileNetworkTimeoutError) { - // If a download was requested, then we can't use a cached copy - if r.download { - return nil, &errors.TaskfileNetworkTimeoutError{URI: node.Location(), Timeout: r.timeout} - } - // Search for any cached copies - if b, err = cache.read(node); errors.Is(err, os.ErrNotExist) { - return nil, &errors.TaskfileNetworkTimeoutError{URI: node.Location(), Timeout: r.timeout, CheckedCache: true} - } else if err != nil { - return nil, err - } - r.logger.VerboseOutf(logger.Magenta, "task: [%s] Network timeout. Fetched cached copy\n", node.Location()) + + // If a download was requested, then we can't use a cached copy + if r.download { + return nil, &errors.TaskfileNetworkTimeoutError{URI: node.Location(), Timeout: r.timeout} + } + + // Search for any cached copies + cached, err := cache.read(node) + if errors.Is(err, os.ErrNotExist) { + return nil, &errors.TaskfileNetworkTimeoutError{URI: node.Location(), Timeout: r.timeout, CheckedCache: true} } else if err != nil { return nil, err - } else { - downloaded = true } + r.logger.VerboseOutf(logger.Magenta, "task: [%s] Network timeout. Fetched cached copy\n", node.Location()) - // If the node was remote, we need to check the checksum - if node.Remote() && downloaded { - r.logger.VerboseOutf(logger.Magenta, "task: [%s] Fetched remote copy\n", node.Location()) - - // Get the checksums - checksum := checksum(b) - cachedChecksum := cache.readChecksum(node) - - var prompt string - if cachedChecksum == "" { - // If the checksum doesn't exist, prompt the user to continue - prompt = fmt.Sprintf(taskfileUntrustedPrompt, node.Location()) - } else if checksum != cachedChecksum { - // If there is a cached hash, but it doesn't match the expected hash, prompt the user to continue - prompt = fmt.Sprintf(taskfileChangedPrompt, node.Location()) - } + return cached, nil - if prompt != "" { - if err := func() error { - r.promptMutex.Lock() - defer r.promptMutex.Unlock() - return r.logger.Prompt(logger.Yellow, prompt, "n", "y", "yes") - }(); err != nil { - return nil, &errors.TaskfileNotTrustedError{URI: node.Location()} - } - } - // If the hash has changed (or is new) - if checksum != cachedChecksum { - // Store the checksum - if err := cache.writeChecksum(node, checksum); err != nil { - return nil, err - } - // Cache the file - r.logger.VerboseOutf(logger.Magenta, "task: [%s] Caching downloaded file\n", node.Location()) - if err = cache.write(node, b); err != nil { - return nil, err - } - } + } else if err != nil { + return nil, err + } + r.logger.VerboseOutf(logger.Magenta, "task: [%s] Fetched remote copy\n", node.Location()) + + // Get the checksums + checksum := checksum(b) + cachedChecksum := cache.readChecksum(node) + + var prompt string + if cachedChecksum == "" { + // If the checksum doesn't exist, prompt the user to continue + prompt = fmt.Sprintf(taskfileUntrustedPrompt, node.Location()) + } else if checksum != cachedChecksum { + // If there is a cached hash, but it doesn't match the expected hash, prompt the user to continue + prompt = fmt.Sprintf(taskfileChangedPrompt, node.Location()) + } + + if prompt != "" { + if err := func() error { + r.promptMutex.Lock() + defer r.promptMutex.Unlock() + return r.logger.Prompt(logger.Yellow, prompt, "n", "y", "yes") + }(); err != nil { + return nil, &errors.TaskfileNotTrustedError{URI: node.Location()} + } + + // Store the checksum + if err := cache.writeChecksum(node, checksum); err != nil { + return nil, err + } + + // Cache the file + r.logger.VerboseOutf(logger.Magenta, "task: [%s] Caching downloaded file\n", node.Location()) + if err = cache.write(node, b); err != nil { + return nil, err } } From 65aad38d6d121c948d52f81be4f287a8eb7e30dd Mon Sep 17 00:00:00 2001 From: Paulo Bittencourt Date: Thu, 17 Oct 2024 17:06:36 -0400 Subject: [PATCH 4/4] fix: use `math/rand/v2` package from Go 1.22 This new package pre-seeds the global rand source on every process start so we don't need to maintain our own rand.Source variable. --- task_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/task_test.go b/task_test.go index e90d8074d6..15d6fd2d8d 100644 --- a/task_test.go +++ b/task_test.go @@ -6,7 +6,7 @@ import ( "fmt" "io" "io/fs" - "math/rand" + rand "math/rand/v2" "net/http" "net/http/httptest" "os" @@ -30,8 +30,6 @@ import ( "github.com/go-task/task/v3/taskfile/ast" ) -var random = rand.New(rand.NewSource(time.Now().UnixNano())) - func init() { _ = os.Setenv("NO_COLOR", "1") } @@ -1127,7 +1125,7 @@ func TestIncludesRemote(t *testing.T) { for k, task := range tasks { t.Run(task, func(t *testing.T) { - expectedContent := fmt.Sprint(random.Int63()) + expectedContent := fmt.Sprint(rand.Int64()) t.Setenv("CONTENT", expectedContent) outputFile := fmt.Sprintf("%d.%d.txt", i, k)