Merge pull request #1518 from buildkite/revert-1504-artifact-download…

…er-destination-trailing-slash

Revert "ArtifactDownloader: retain destination trailing slash"

agent/artifact_downloader.go

@@ -52,20 +52,9 @@ func NewArtifactDownloader(l logger.Logger, ac APIClient, c ArtifactDownloaderCo
 	}
 }
 
-func getDownloadDestination(destination string) string {
-	// Filepath.Abs calls filepath.Clean on the result, which removes any
-	// trailing forward slash. We will then retain any trailing forward slash
-	// as it may indicate path merging behavior for download.getTargetPath.
-	downloadDestination, _ := filepath.Abs(destination)
-	if strings.HasSuffix(destination, string(os.PathSeparator)) && len(destination) > 1 {
-		downloadDestination += string(os.PathSeparator)
-	}
-	return downloadDestination
-}
-
 func (a *ArtifactDownloader) Download() error {
 	// Turn the download destination into an absolute path and confirm it exists
-	downloadDestination := getDownloadDestination(a.conf.Destination)
+	downloadDestination, _ := filepath.Abs(a.conf.Destination)
 	fileInfo, err := os.Stat(downloadDestination)
 	if err != nil {
 		return fmt.Errorf("Could not find information about destination: %s %v",

agent/artifact_downloader_test.go

@@ -5,12 +5,10 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
-	"path/filepath"
 	"testing"
 
 	"github.com/buildkite/agent/v3/api"
 	"github.com/buildkite/agent/v3/logger"
-	"github.com/stretchr/testify/assert"
 )
 
 func TestArtifactDownloaderConnectsToEndpoint(t *testing.T) {
@@ -48,14 +46,3 @@ func TestArtifactDownloaderConnectsToEndpoint(t *testing.T) {
 		t.Fatal(err)
 	}
 }
-
-func TestGetDownloadDestination(t *testing.T) {
-	workingDirectory, _ := filepath.Abs(".")
-	assert.Equal(t, workingDirectory + string(os.PathSeparator) + "a", getDownloadDestination("a"))
-	assert.Equal(t, workingDirectory + string(os.PathSeparator) + "a" + string(os.PathSeparator), getDownloadDestination("a" + string(os.PathSeparator)))
-
-	// Test that we don't get a double // on unix, must use filepath.Abs
-	// to handle the Windows case which normalises to C:\
-	root, _ := filepath.Abs("/")
-	assert.Equal(t, root, getDownloadDestination("/"))
-}

clicommand/artifact_download.go

@@ -26,11 +26,6 @@ Description:
    'foo/app' will write any matched artifact files to 'foo/app/logs/', relative
    to the current working directory.
 
-   To avoid this behaviour, use a <destination> argument with a trailing slash.
-   For example, a query of 'app/logs/*' and a destination of 'foo/app/' will
-   write the matched artifact files to 'foo/app/app/logs/', relative to the
-   current working directory.
-
    You can also change working directory to the intended destination and use a
    <destination> of '.' to always create a directory hierarchy matching the
    artifact paths.