Skip to content
This repository has been archived by the owner on Sep 17, 2024. It is now read-only.

Update fetchBeatsBinary to be reused in elastic-agent-poc #1984

Merged
merged 10 commits into from
Jan 17, 2022
+101 −58
Merged

Update fetchBeatsBinary to be reused in elastic-agent-poc #1984

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
34d8a97
update func
narph Jan 10, 2022
4b938d4
fix path
narph Jan 10, 2022
ff34138
Merge branch 'main' into download-path
narph Jan 10, 2022
84b0864
work on download
narph Jan 11, 2022
2f19ed7
small fix
narph Jan 11, 2022
6596b0b
remove test
narph Jan 11, 2022
2296bf5
add sha to google
narph Jan 11, 2022
fb2eda2
Merge branch 'main' into download-path
narph Jan 12, 2022
b3778ed
fix typo
narph Jan 17, 2022
2e3ecea
add comment
narph Jan 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions internal/utils/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ func GetArchitecture() string {
// DownloadFile will download a url and store it in a temporary path.
// It writes to the destination file as it downloads it, without
// loading the entire file into memory.
func DownloadFile(url string) (string, error) {
func DownloadFile(url string, downloadPath string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking at the implementation and it's not clear to me if we need to return the path again, as it's passed in the download path. I can picture a new struct (i.e. DownloadRequest) with two attributes: url and downloadPath. The DownloadFile method would receive a pointer to an instance of this struct. If the downloadPath is empty, then create a temporary file (as it does now), populating the download path, otherwise use the value in the request. The method would still return the request's download path.

Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea if we're already passing in the downloadPath then a struct containing that information could be easily reused/referenced elsewhere. I also tend to go with defined structs as it improves readability and intent

var filepathFull string
tempParentDir := filepath.Join(os.TempDir(), uuid.NewString())
internalio.MkdirAll(tempParentDir)

Expand All @@ -54,7 +55,11 @@ func DownloadFile(url string) (string, error) {
}
defer tempFile.Close()

filepathFull := tempFile.Name()
if downloadPath != "" {
filepathFull = downloadPath
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move https://github.com/elastic/e2e-testing/pull/1984/files#diff-a7cadd8b7616e655fd258138320b33eb96f7cce8e8663ec92651f49ce07df000R45-R46 right here, covered by the else branch? We will be creating the temp file if and only if the downloadPath is not empty

filepathFull = tempFile.Name()
}

exp := GetExponentialBackOff(3)

Expand Down
2 changes: 1 addition & 1 deletion internal/utils/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
)

func TestDownloadFile(t *testing.T) {
f, err := DownloadFile("https://www.elastic.co/robots.txt")
f, err := DownloadFile("https://www.elastic.co/robots.txt", "")
assert.Nil(t, err)
defer os.Remove(filepath.Dir(f))
}
Expand Down
8 changes: 4 additions & 4 deletions pkg/downloads/versions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func CheckPRVersion(version string, fallbackVersion string) string {
// FetchElasticArtifact fetches an artifact from the right repository, returning binary name, path and error
func FetchElasticArtifact(ctx context.Context, artifact string, version string, os string, arch string, extension string, isDocker bool, xpack bool) (string, string, error) {
binaryName := buildArtifactName(artifact, version, os, arch, extension, isDocker)
binaryPath, err := fetchBeatsBinary(ctx, binaryName, artifact, version, utils.TimeoutFactor, xpack)
binaryPath, err := FetchBeatsBinary(ctx, binaryName, artifact, version, utils.TimeoutFactor, xpack,"")
narph marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
log.WithFields(log.Fields{
"artifact": artifact,
Expand Down Expand Up @@ -347,12 +347,12 @@ func buildArtifactName(artifact string, artifactVersion string, OS string, arch

}

// fetchBeatsBinary it downloads the binary and returns the location of the downloaded file
// FetchBeatsBinary it downloads the binary and returns the location of the downloaded file
// If the environment variable BEATS_LOCAL_PATH is set, then the artifact
// to be used will be defined by the local snapshot produced by the local build.
// Else, if the environment variable GITHUB_CHECK_SHA1 is set, then the artifact
// to be downloaded will be defined by the snapshot produced by the Beats CI for that commit.
func fetchBeatsBinary(ctx context.Context, artifactName string, artifact string, version string, timeoutFactor int, xpack bool) (string, error) {
func FetchBeatsBinary(ctx context.Context, artifactName string, artifact string, version string, timeoutFactor int, xpack bool, downloadPath string) (string, error) {
if BeatsLocalPath != "" {
span, _ := apm.StartSpanOptions(ctx, "Fetching Beats binary", "beats.local.fetch-binary", apm.SpanOptions{
Parent: apm.SpanFromContext(ctx).TraceContext(),
Expand Down Expand Up @@ -389,7 +389,7 @@ func fetchBeatsBinary(ctx context.Context, artifactName string, artifact string,
return val, nil
}

filePathFull, err := utils.DownloadFile(URL)
filePathFull, err := utils.DownloadFile(URL, downloadPath)
if err != nil {
return filePathFull, err
}
Expand Down
24 changes: 12 additions & 12 deletions pkg/downloads/versions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ func TestFetchBeatsBinaryFromLocalPath(t *testing.T) {
defer func() { BeatsLocalPath = "" }()
BeatsLocalPath = beatsDir

_, err := fetchBeatsBinary(ctx, "foo_fileName", artifact, version, utils.TimeoutFactor, true)
_, err := FetchBeatsBinary(ctx, "foo_fileName", artifact, version, utils.TimeoutFactor, true,"")
narph marked this conversation as resolved.
Show resolved Hide resolved
assert.NotNil(t, err)
})

Expand All @@ -398,7 +398,7 @@ func TestFetchBeatsBinaryFromLocalPath(t *testing.T) {
artifactName := versionPrefix + "-x86_64.rpm"
expectedFilePath := path.Join(distributionsDir, artifactName)

downloadedFilePath, err := fetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true)
downloadedFilePath, err := FetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true,"")
narph marked this conversation as resolved.
Show resolved Hide resolved
assert.Nil(t, err)
assert.Equal(t, downloadedFilePath, expectedFilePath)
})
Expand All @@ -409,7 +409,7 @@ func TestFetchBeatsBinaryFromLocalPath(t *testing.T) {
artifactName := versionPrefix + "-aarch64.rpm"
expectedFilePath := path.Join(distributionsDir, artifactName)

downloadedFilePath, err := fetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true)
downloadedFilePath, err := FetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true,"")
narph marked this conversation as resolved.
Show resolved Hide resolved
assert.Nil(t, err)
assert.Equal(t, downloadedFilePath, expectedFilePath)
})
Expand All @@ -421,7 +421,7 @@ func TestFetchBeatsBinaryFromLocalPath(t *testing.T) {
artifactName := versionPrefix + "-amd64.deb"
expectedFilePath := path.Join(distributionsDir, artifactName)

downloadedFilePath, err := fetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true)
downloadedFilePath, err := FetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true,"")
narph marked this conversation as resolved.
Show resolved Hide resolved
assert.Nil(t, err)
assert.Equal(t, downloadedFilePath, expectedFilePath)
})
Expand All @@ -432,7 +432,7 @@ func TestFetchBeatsBinaryFromLocalPath(t *testing.T) {
artifactName := versionPrefix + "-arm64.deb"
expectedFilePath := path.Join(distributionsDir, artifactName)

downloadedFilePath, err := fetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true)
downloadedFilePath, err := FetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true,"")
narph marked this conversation as resolved.
Show resolved Hide resolved
assert.Nil(t, err)
assert.Equal(t, downloadedFilePath, expectedFilePath)
})
Expand All @@ -444,7 +444,7 @@ func TestFetchBeatsBinaryFromLocalPath(t *testing.T) {
artifactName := versionPrefix + "-linux-amd64.tar.gz"
expectedFilePath := path.Join(distributionsDir, artifactName)

downloadedFilePath, err := fetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true)
downloadedFilePath, err := FetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true,"")
narph marked this conversation as resolved.
Show resolved Hide resolved
assert.Nil(t, err)
assert.Equal(t, downloadedFilePath, expectedFilePath)
})
Expand All @@ -455,7 +455,7 @@ func TestFetchBeatsBinaryFromLocalPath(t *testing.T) {
artifactName := versionPrefix + "-linux-x86_64.tar.gz"
expectedFilePath := path.Join(distributionsDir, artifactName)

downloadedFilePath, err := fetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true)
downloadedFilePath, err := FetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true,"")
narph marked this conversation as resolved.
Show resolved Hide resolved
assert.Nil(t, err)
assert.Equal(t, downloadedFilePath, expectedFilePath)
})
Expand All @@ -466,7 +466,7 @@ func TestFetchBeatsBinaryFromLocalPath(t *testing.T) {
artifactName := versionPrefix + "-linux-arm64.tar.gz"
expectedFilePath := path.Join(distributionsDir, artifactName)

downloadedFilePath, err := fetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true)
downloadedFilePath, err := FetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true,"")
narph marked this conversation as resolved.
Show resolved Hide resolved
assert.Nil(t, err)
assert.Equal(t, downloadedFilePath, expectedFilePath)
})
Expand All @@ -478,7 +478,7 @@ func TestFetchBeatsBinaryFromLocalPath(t *testing.T) {
artifactName := versionPrefix + "-linux-amd64.docker.tar.gz"
expectedFilePath := path.Join(distributionsDir, artifactName)

downloadedFilePath, err := fetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true)
downloadedFilePath, err := FetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true,"")
narph marked this conversation as resolved.
Show resolved Hide resolved
assert.Nil(t, err)
assert.Equal(t, downloadedFilePath, expectedFilePath)
})
Expand All @@ -489,7 +489,7 @@ func TestFetchBeatsBinaryFromLocalPath(t *testing.T) {
artifactName := versionPrefix + "-linux-arm64.docker.tar.gz"
expectedFilePath := path.Join(distributionsDir, artifactName)

downloadedFilePath, err := fetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true)
downloadedFilePath, err := FetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true,"")
narph marked this conversation as resolved.
Show resolved Hide resolved
assert.Nil(t, err)
assert.Equal(t, downloadedFilePath, expectedFilePath)
})
Expand All @@ -501,7 +501,7 @@ func TestFetchBeatsBinaryFromLocalPath(t *testing.T) {
artifactName := ubi8VersionPrefix + "-linux-amd64.docker.tar.gz"
expectedFilePath := path.Join(distributionsDir, artifactName)

downloadedFilePath, err := fetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true)
downloadedFilePath, err := FetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true,"")
narph marked this conversation as resolved.
Show resolved Hide resolved
assert.Nil(t, err)
assert.Equal(t, downloadedFilePath, expectedFilePath)
})
Expand All @@ -512,7 +512,7 @@ func TestFetchBeatsBinaryFromLocalPath(t *testing.T) {
artifactName := ubi8VersionPrefix + "-linux-arm64.docker.tar.gz"
expectedFilePath := path.Join(distributionsDir, artifactName)

downloadedFilePath, err := fetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true)
downloadedFilePath, err := FetchBeatsBinary(ctx, artifactName, artifact, version, utils.TimeoutFactor, true,"")
narph marked this conversation as resolved.
Show resolved Hide resolved
assert.Nil(t, err)
assert.Equal(t, downloadedFilePath, expectedFilePath)
})
Expand Down