Args: Add path arg type to fix working directory handling (#442)

Summary:
Pull Request resolved: #442

* Add new argument type `path:`
* This makes relative paths work correctly when passed as arguments
* Previously, if you passed `--arg myarg=foo/bar` the behavior was busted because the TTP will change its working directory to the parent directory of the  TTP's YAML file, and `foo/bar` won't exist there, even if it existed in the directory from which you invoked ttpforge :(
* Updated all examples to use the new arguments

Reviewed By: d0n601

Differential Revision: D51475492

fbshipit-source-id: 4dea0a1b0415ab39580ace902e3bd6e75aab315e

cmd/run_test.go

@@ -21,26 +21,50 @@ package cmd
 
 import (
 	"bytes"
+	"os"
 	"path/filepath"
 
 	"testing"
 
+	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+const (
+	testResourcesDir = "test-resources"
+	testRepoName     = "test-repo"
+)
+
+type runCmdTestCase struct {
+	name           string
+	description    string
+	args           []string
+	expectedStdout string
+	wantError      bool
+}
+
+func checkRunCmdTestCase(t *testing.T, tc runCmdTestCase) {
+	var stdoutBuf, stderrBuf bytes.Buffer
+	rc := BuildRootCommand(&TestConfig{
+		Stdout: &stdoutBuf,
+		Stderr: &stderrBuf,
+	})
+	rc.SetArgs(append([]string{"run"}, tc.args...))
+	err := rc.Execute()
+	if tc.wantError {
+		require.Error(t, err)
+		return
+	}
+	require.NoError(t, err)
+	assert.Equal(t, tc.expectedStdout, stdoutBuf.String())
+
+}
+
 func TestRun(t *testing.T) {
-	const testResourcesDir = "test-resources"
-	const testRepoName = "test-repo"
 	testConfigFilePath := filepath.Join(testResourcesDir, "test-config.yaml")
 
-	testCases := []struct {
-		name           string
-		description    string
-		args           []string
-		expectedStdout string
-		wantError      bool
-	}{
+	testCases := []runCmdTestCase{
 		{
 			name:        "file-step",
 			description: "check that a regular file step works",
@@ -127,19 +151,89 @@ func TestRun(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			var stdoutBuf, stderrBuf bytes.Buffer
-			rc := BuildRootCommand(&TestConfig{
-				Stdout: &stdoutBuf,
-				Stderr: &stderrBuf,
-			})
-			rc.SetArgs(append([]string{"run"}, tc.args...))
-			err := rc.Execute()
-			if tc.wantError {
-				require.Error(t, err)
-			} else {
-				require.NoError(t, err)
-			}
-			assert.Equal(t, tc.expectedStdout, stdoutBuf.String())
+			checkRunCmdTestCase(t, tc)
+		})
+	}
+}
+
+// TestRunPathArguments checks that
+// referencing relative paths in `--arg` values
+// when executing `ttpforge run` works as expected.
+// One typically needs to specify `type: path` in
+// the argument specificiation in order to get desired
+// behavior.
+func TestRunPathArguments(t *testing.T) {
+	// in this test, we initially execute every test case from a
+	// temporary directory, so we need the absolute path to the config
+	testConfigFilePath, err := filepath.Abs(filepath.Join(testResourcesDir, "test-config.yaml"))
+	require.NoError(t, err)
+	// the file we will read from in the test cases
+	targetFileName := "path-test.txt"
+
+	// setup dummy file(s) for our test cases
+	fsys := afero.NewOsFs()
+	targetDir, err := afero.TempDir(fsys, "", "ttpforge-run-test")
+	targetAbsPath := filepath.Join(targetDir, targetFileName)
+	require.NoError(t, err)
+	f, err := fsys.Create(targetAbsPath)
+	require.NoError(t, err)
+	defer fsys.Remove(targetFileName)
+	f.Write([]byte("It worked!\n"))
+
+	testCases := []runCmdTestCase{
+		{
+			name:        "Argument with `type: path` - Should Fail",
+			description: "This should fail because the TTP's argument spec does not have `type: path`",
+			args: []string{
+				"-c",
+				testConfigFilePath,
+				testRepoName + "//args/path/without-path.yaml",
+				"--arg",
+				"target_path=" + targetFileName,
+			},
+			wantError: true,
+		},
+		{
+			name:        "Argument with `type: path` - Should Succeed",
+			description: "This should succeed because the TTP's argument spec has `type: path`",
+			args: []string{
+				"-c",
+				testConfigFilePath,
+				testRepoName + "//args/path/with-path.yaml",
+				"--arg",
+				"target_path=" + targetFileName,
+			},
+			expectedStdout: "It worked!\n",
+		},
+		{
+			name:        "Argument with `type: path` - Absolute Path - Should Succeed",
+			description: "Extra test case to check that `type: path` works with absolute paths",
+			args: []string{
+				"-c",
+				testConfigFilePath,
+				testRepoName + "//args/path/with-path.yaml",
+				"--arg",
+				"target_path=" + targetAbsPath,
+			},
+			expectedStdout: "It worked!\n",
+		},
+	}
+
+	// change directory to the target dir so we can use relative paths
+	// to refer to the target file
+	wd, err := os.Getwd()
+	require.NoError(t, err)
+	err = os.Chdir(targetDir)
+	require.NoError(t, err)
+	defer func() {
+		if err := os.Chdir(wd); err != nil {
+			panic(err)
+		}
+	}()
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			checkRunCmdTestCase(t, tc)
 		})
 	}
 }

cmd/test-resources/repos/test-repo/ttps/args/path/with-path.yaml

@@ -0,0 +1,15 @@
+name: Testing Path Type Arguments
+description: |
+  This TTP powers the test case
+  "Argument with `type: path` - Should Succeed"
+  in `cmd/run_test.go`
+args:
+  - name: target_path
+    description: |
+      If a relative path is provided for this argument, it will
+      be expanded to an absolute path based on the user's current
+      working directory, NOT the configuration directory of the TTP.
+    type: path
+steps:
+  - name: cat_target_path
+    inline: cat {{.Args.target_path}}

cmd/test-resources/repos/test-repo/ttps/args/path/without-path.yaml

@@ -0,0 +1,14 @@
+name: Testing Path Type Arguments - Failure
+description: |
+  This TTP powers the test case
+  "Argument with `type: path` - Should Fail"
+  in `cmd/run_test.go`
+args:
+  - name: target_path
+    description: |
+      If a valid relative path is provided for this argument, the TTP
+      will still fail because it will run from the TTP's configuration
+      directory and that path won't exist there
+steps:
+  - name: cat_target_path
+    inline: cat {{.Args.target_path}}

example-ttps/actions/edit_file/append_delete.yaml

@@ -5,6 +5,7 @@ description: |
   with the edit_file action.
 args:
   - name: test_file_path
+    type: path
     description: The path at which the temporary test file should be created
     default: /tmp/ttpforge_edit_file_append_delete
 steps:

example-ttps/actions/edit_file/replace.yaml

@@ -6,6 +6,7 @@ description: |
   or regexp matches with new strings
 args:
   - name: test_file_path
+    type: path
     description: The path at which the temporary test file should be created
     default: /tmp/ttpforge_edit_file_replace
 steps:

example-ttps/introduction/dotfile-backdoor-demo.yaml

@@ -6,11 +6,16 @@ description: |
     - Simple but powerful Command-line Argument Support
     - Last-in-First-Out Cleanup Execution
     - Checking Conditions at Runtime to Avoid Errors
+tests:
+  - name: default
+    description: execute with the default setting
 args:
   - name: target_file_path
+    type: path
     description: The file that we should try to backdoor
     default: ~/.zshrc
   - name: payload_file_path
+    type: path
     description: |
       The path to which we should write the payload file.
       The backdoor we insert into the target file will reference this
@@ -21,6 +26,7 @@ args:
       The shell command that our payload should execute
     default: echo 'Hello from TTPForge! You have been pwned!'
   - name: backup_file_path
+    type: path
     description: |
       The file path to which the target file should be backed up
     default: /tmp/ttpforge-dotfile-backdoor-backup

example-ttps/tests/with_args.yaml

@@ -22,6 +22,7 @@ tests:
       contents: this will not be printed
 args:
   - name: target_file_path
+    type: path
     description: the path of the file to create
   - name: contents
     description: the contents to write to the target file

pkg/args/path_test.go

@@ -0,0 +1,109 @@
+/*
+Copyright © 2023-present, Meta Platforms, Inc. and affiliates
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+The above copyright notice and this permission notice shall be included in
+all copies or substantial portions of the Software.
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+THE SOFTWARE.
+*/
+
+package args
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestValidateArgsPath(t *testing.T) {
+
+	// used in some test cases
+	homedir, err := os.UserHomeDir()
+	require.NoError(t, err)
+
+	// cd into this directory and use it to verify
+	// that relative paths are expanded appropriately
+	tmpDir, err := os.MkdirTemp("", "ttpforge-testing")
+	require.NoError(t, err)
+	defer os.RemoveAll(tmpDir)
+	wd, err := os.Getwd()
+	require.NoError(t, err)
+	err = os.Chdir(tmpDir)
+	require.NoError(t, err)
+	defer func() {
+		// this will break a bunch of other tests so be loud about it
+		if err := os.Chdir(wd); err != nil {
+			panic(err)
+		}
+	}()
+	// macOS /private/var/...  shenanigans
+	tmpDir, err = filepath.EvalSymlinks(tmpDir)
+	require.NoError(t, err)
+
+	testCases := []validateTestCase{
+		{
+			name: "Absolute Path",
+			specs: []Spec{
+				{
+					Name: "mypath",
+					Type: "path",
+				},
+			},
+			argKvStrs: []string{
+				"mypath=/tmp/foo",
+			},
+			expectedResult: map[string]any{
+				"mypath": "/tmp/foo",
+			},
+		},
+		{
+			name: "Path Containing Tilde",
+			specs: []Spec{
+				{
+					Name: "mypath",
+					Type: "path",
+				},
+			},
+			argKvStrs: []string{
+				"mypath=~/wut",
+			},
+			expectedResult: map[string]any{
+				"mypath": filepath.Join(homedir, "wut"),
+			},
+		},
+		{
+			name: "Relative Path",
+			specs: []Spec{
+				{
+					Name: "mypath",
+					Type: "path",
+				},
+			},
+			argKvStrs: []string{
+				"mypath=" + filepath.Join("foo", "bar"),
+			},
+			// can't do a strict equality 0
+			expectedResult: map[string]any{
+				"mypath": filepath.Join(tmpDir, "foo", "bar"),
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			checkValidateTestCase(t, tc)
+		})
+	}
+}

pkg/args/spec.go

@@ -25,6 +25,8 @@ import (
 	"regexp"
 	"strconv"
 	"strings"
+
+	"github.com/facebookincubator/ttpforge/pkg/fileutils"
 )
 
 // Spec defines a CLI argument for the TTP
@@ -161,6 +163,12 @@ func (spec Spec) convertArgToType(val string) (any, error) {
 			return nil, errors.New("no-boolean value provided")
 		}
 		return asBool, nil
+	case "path":
+		absPath, err := fileutils.AbsPath(val)
+		if err != nil {
+			return nil, fmt.Errorf("failed to process argument of type `path`: %w", err)
+		}
+		return absPath, nil
 	default:
 		return nil, fmt.Errorf("invalid type %v specified in configuration for argument %v", spec.Type, spec.Name)
 	}

pkg/fileutils/fileutils.go

@@ -36,3 +36,13 @@ func ExpandTilde(path string) (string, error) {
 	}
 	return path, nil
 }
+
+// AbsPath is a thin wrapper around filepath.Abs
+// that we use because it also calls ExpandTilde
+func AbsPath(path string) (string, error) {
+	tmp, err := ExpandTilde(path)
+	if err != nil {
+		return "", err
+	}
+	return filepath.Abs(tmp)
+}