Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update lifecycle.DownloadBinary to work with local file #5398

Merged
merged 1 commit into from
Dec 9, 2024
Merged

Update lifecycle.DownloadBinary to work with local file #5398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
53 changes: 37 additions & 16 deletions pkg/lifecycle/binary.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
"fmt"
"io"
"net/http"
"net/url"
"os"
"os/exec"
"path/filepath"
Expand Down Expand Up @@ -104,7 +105,7 @@

// DownloadBinary downloads a file from the given URL into the specified path
// this also marks it executable and returns its full path.
func DownloadBinary(url, destDir, destFile string, logger *zap.Logger) (string, error) {
func DownloadBinary(sourceURL, destDir, destFile string, logger *zap.Logger) (string, error) {
if err := os.MkdirAll(destDir, 0755); err != nil {
return "", fmt.Errorf("could not create directory %s (%w)", destDir, err)
}
Expand Down Expand Up @@ -132,32 +133,52 @@
}
}()

logger.Info("downloading binary", zap.String("url", url))
logger.Info("downloading binary", zap.String("url", sourceURL))

req, err := http.NewRequest("GET", url, nil)
u, err := url.Parse(sourceURL)
if err != nil {
return "", fmt.Errorf("could not create request (%w)", err)
return "", fmt.Errorf("could not parse URL %s (%w)", sourceURL, err)

Check warning on line 140 in pkg/lifecycle/binary.go

View check run for this annotation

Codecov / codecov/patch

pkg/lifecycle/binary.go#L140 

Added line #L140 was not covered by tests
}
client := &http.Client{}
resp, err := client.Do(req)
if err != nil {
return "", fmt.Errorf("HTTP GET %s failed (%w)", url, err)
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
return "", fmt.Errorf("HTTP GET %s failed with error %d", url, resp.StatusCode)
}
switch u.Scheme {
case "http", "https":
Copy link
Member Author

@khanhtc1202 khanhtc1202 Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This makes the test easier since I don't want to mock the HTTPS server. The validation logic in the piped configuration remains to only allow the "https" and "file" schemes.

Copy link
Member

Choose a reason for hiding this comment

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

📝 It is difficult to test with https server because DownloadBinary uses http.Client{} internally and can't inject for now. So we can't use httptest.NewTLSServer.

Copy link
Member Author

Choose a reason for hiding this comment

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

@t-kikuc For the question you asked 🙏

Copy link
Member

Choose a reason for hiding this comment

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

@khanhtc1202

I got it.
binary.go allows http and https, PipedPlugin disallows http.

pipecd/pkg/configv1/piped.go

Lines 1328 to 1330 in dcecc2d

if u.Scheme != "file" && u.Scheme != "https" {
return errors.New("only file and https schemes are supported")
}

Copy link
Member

Choose a reason for hiding this comment

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

[ASK] Are we gonna support http too? or only https?

Copy link
Member Author

Choose a reason for hiding this comment

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

No we don't, just place http there to make testing process simpler 🙏

req, err := http.NewRequest("GET", sourceURL, nil)
if err != nil {
return "", fmt.Errorf("could not create request (%w)", err)
}

Check warning on line 148 in pkg/lifecycle/binary.go

View check run for this annotation

Codecov / codecov/patch

pkg/lifecycle/binary.go#L147-L148 

Added lines #L147 - L148 were not covered by tests
client := &http.Client{}
resp, err := client.Do(req)
if err != nil {
return "", fmt.Errorf("HTTP GET %s failed (%w)", sourceURL, err)
}

Check warning on line 153 in pkg/lifecycle/binary.go

View check run for this annotation

Codecov / codecov/patch

pkg/lifecycle/binary.go#L152-L153 

Added lines #L152 - L153 were not covered by tests
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
return "", fmt.Errorf("HTTP GET %s failed with error %d", sourceURL, resp.StatusCode)
}

Check warning on line 158 in pkg/lifecycle/binary.go

View check run for this annotation

Codecov / codecov/patch

pkg/lifecycle/binary.go#L157-L158 

Added lines #L157 - L158 were not covered by tests

if _, err = io.Copy(tmpFile, resp.Body); err != nil {
return "", fmt.Errorf("could not copy from %s to %s (%w)", url, tmpName, err)
if _, err = io.Copy(tmpFile, resp.Body); err != nil {
return "", fmt.Errorf("could not copy from %s to %s (%w)", sourceURL, tmpName, err)
}

Check warning on line 162 in pkg/lifecycle/binary.go

View check run for this annotation

Codecov / codecov/patch

pkg/lifecycle/binary.go#L161-L162 

Added lines #L161 - L162 were not covered by tests

case "file":
data, err := os.ReadFile(u.Path)
if err != nil {
return "", fmt.Errorf("could not read file %s (%w)", u.Path, err)
}

Check warning on line 168 in pkg/lifecycle/binary.go

View check run for this annotation

Codecov / codecov/patch

pkg/lifecycle/binary.go#L167-L168 

Added lines #L167 - L168 were not covered by tests

if _, err = tmpFile.Write(data); err != nil {
return "", fmt.Errorf("could not write to %s (%w)", tmpName, err)
}

Check warning on line 172 in pkg/lifecycle/binary.go

View check run for this annotation

Codecov / codecov/patch

pkg/lifecycle/binary.go#L171-L172 

Added lines #L171 - L172 were not covered by tests

default:
return "", fmt.Errorf("unsupported file scheme %s", u.Scheme)
}

if err := os.Chmod(tmpName, 0755); err != nil {
return "", fmt.Errorf("could not chmod file %s (%w)", tmpName, err)
}

tmpFile.Close()
if err := os.Rename(tmpName, destPath); err != nil {
return "", fmt.Errorf("could not move %s to %s (%w)", tmpName, destPath, err)
}
Expand Down
37 changes: 35 additions & 2 deletions pkg/lifecycle/binary_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import (
"context"
"net/http"
"net/http/httptest"
"os"
"path"
"strconv"
"testing"
"time"
Expand Down Expand Up @@ -91,17 +93,19 @@ func TestDownloadBinary(t *testing.T) {
defer server.Close()

logger := zaptest.NewLogger(t)
destDir := t.TempDir()
destFile := "test-binary"

t.Run("successful download", func(t *testing.T) {
destDir := t.TempDir()
destFile := "test-binary"
url := server.URL + "/binary"
path, err := DownloadBinary(url, destDir, destFile, logger)
require.NoError(t, err)
assert.FileExists(t, path)
})

t.Run("file already exists", func(t *testing.T) {
destDir := t.TempDir()
destFile := "test-binary"
url := server.URL + "/binary"
path, err := DownloadBinary(url, destDir, destFile, logger)
require.NoError(t, err)
Expand All @@ -112,6 +116,35 @@ func TestDownloadBinary(t *testing.T) {
require.NoError(t, err)
assert.FileExists(t, path)
})

t.Run("file on local", func(t *testing.T) {
sourceDir := t.TempDir()
sourceFile := "test-binary"
sourcePath := path.Join(sourceDir, sourceFile)
err := os.WriteFile(sourcePath, []byte("test binary content"), 0755)
require.NoError(t, err)

destDir := t.TempDir()
destFile := "test-binary"
url := "file://" + sourcePath

path, err := DownloadBinary(url, destDir, destFile, logger)
require.NoError(t, err)
assert.FileExists(t, path)
content, err := os.ReadFile(path)
require.NoError(t, err)
assert.Equal(t, "test binary content", string(content))
})

t.Run("not valid source url given", func(t *testing.T) {
destDir := t.TempDir()
destFile := "test-binary"
url := "ftp://invalid-url"

path, err := DownloadBinary(url, destDir, destFile, logger)
require.Error(t, err)
assert.Empty(t, path)
})
}

func httpTestServer() *httptest.Server {
Expand Down
Loading