Skip to content

Commit

Permalink
refactor fileinfo
Browse files Browse the repository at this point in the history 
not closing a file was causing windows tests to fail
in pivnet cli

instead of passing around a file, pass around a
fileInfo

a custom file info was created because the native
file info does not use absolute path for its name

[#159890068]

Signed-off-by: Ka Hin Ng <[email protected]>
Signed-off-by: Paul Nikonowicz <[email protected]>
  • Loading branch information
Paul Nikonowicz committed Feb 28, 2019
1 parent a4092f6 commit 2172a68
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 70 deletions.
31 changes: 23 additions & 8 deletions download/downloader.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,27 @@ type Client struct {
Timeout time.Duration
}

type FileInfo struct {
Name string
Mode os.FileMode
}

func NewFileInfo(file *os.File) (*FileInfo, error) {
stat, err := file.Stat()
if err != nil {
return nil, err
}

fileInfo := &FileInfo {
Name: file.Name(),
Mode: stat.Mode(),
}

return fileInfo, nil
}

func (c Client) Get(
location *os.File,
location *FileInfo,
downloadLinkFetcher downloadLinkFetcher,
progressWriter io.Writer,
) error {
Expand Down Expand Up @@ -83,7 +102,7 @@ func (c Client) Get(
return fmt.Errorf("failed to construct range: %s", err)
}

diskStats, err := disk.Usage(path.Dir(location.Name()))
diskStats, err := disk.Usage(path.Dir(location.Name))
if err != nil {
return fmt.Errorf("failed to get disk free space: %s", err)
}
Expand All @@ -97,18 +116,14 @@ func (c Client) Get(
c.Bar.Kickoff()

defer c.Bar.Finish()
fileInfo, err := location.Stat()
if err != nil {
return fmt.Errorf("failed to read information from output file: %s", err)
}

var g errgroup.Group
for _, r := range ranges {
byteRange := r

fileWriter, err := os.OpenFile(location.Name(), os.O_RDWR, fileInfo.Mode())
fileWriter, err := os.OpenFile(location.Name, os.O_RDWR, location.Mode)
if err != nil {
return fmt.Errorf("failed to open file for writing: %s", err)
return fmt.Errorf("failed to open file %s for writing: %s", location.Name, err)
}

g.Go(func() error {
Expand Down
73 changes: 23 additions & 50 deletions download/downloader_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,10 @@ var _ = Describe("Downloader", func() {
tmpFile, err := ioutil.TempFile("", "")
Expect(err).NotTo(HaveOccurred())

err = downloader.Get(tmpFile, downloadLinkFetcher, GinkgoWriter)
tmpLocation, err := download.NewFileInfo(tmpFile)
Expect(err).NotTo(HaveOccurred())

err = downloader.Get(tmpLocation, downloadLinkFetcher, GinkgoWriter)
Expect(err).NotTo(HaveOccurred())

content, err := ioutil.ReadAll(tmpFile)
Expand Down Expand Up @@ -227,7 +230,10 @@ var _ = Describe("Downloader", func() {
tmpFile, err = ioutil.TempFile("", "")
Expect(err).NotTo(HaveOccurred())

err = downloader.Get(tmpFile, downloadLinkFetcher, GinkgoWriter)
tmpLocation, err := download.NewFileInfo(tmpFile)
Expect(err).NotTo(HaveOccurred())

err = downloader.Get(tmpLocation, downloadLinkFetcher, GinkgoWriter)
Expect(err).NotTo(HaveOccurred())
})

Expand Down Expand Up @@ -381,7 +387,10 @@ var _ = Describe("Downloader", func() {
file, err := ioutil.TempFile("", "")
Expect(err).NotTo(HaveOccurred())

err = downloader.Get(file, downloadLinkFetcher, GinkgoWriter)
location, err := download.NewFileInfo(file)
Expect(err).NotTo(HaveOccurred())

err = downloader.Get(location, downloadLinkFetcher, GinkgoWriter)
Expect(err).To(MatchError(ContainSubstring("file is too big to fit on this drive:")))
Expect(err).To(MatchError(ContainSubstring("bytes required")))
Expect(err).To(MatchError(ContainSubstring("bytes free")))
Expand Down Expand Up @@ -423,7 +432,10 @@ var _ = Describe("Downloader", func() {
file, err := ioutil.TempFile("", "")
Expect(err).NotTo(HaveOccurred())

err = downloader.Get(file, downloadLinkFetcher, GinkgoWriter)
location, err := download.NewFileInfo(file)
Expect(err).NotTo(HaveOccurred())

err = downloader.Get(location, downloadLinkFetcher, GinkgoWriter)
Expect(err).To(MatchError(ContainSubstring("failed to find file")))
})
})
Expand Down Expand Up @@ -523,7 +535,10 @@ var _ = Describe("Downloader", func() {
file, err := ioutil.TempFile("", "")
Expect(err).NotTo(HaveOccurred())

err = downloader.Get(file, downloadLinkFetcher, GinkgoWriter)
location, err := download.NewFileInfo(file)
Expect(err).NotTo(HaveOccurred())

err = downloader.Get(location, downloadLinkFetcher, GinkgoWriter)
Expect(err).To(MatchError("problem while waiting for chunks to download: failed during retryable request: download request failed: failed GET"))
})
})
Expand Down Expand Up @@ -565,53 +580,11 @@ var _ = Describe("Downloader", func() {
file, err := ioutil.TempFile("", "")
Expect(err).NotTo(HaveOccurred())

err = downloader.Get(file, downloadLinkFetcher, GinkgoWriter)
Expect(err).To(MatchError("problem while waiting for chunks to download: failed during retryable request: during GET unexpected status code was returned: 500"))
})
})

Context("when the file cannot be written to", func() {
It("returns an error", func() {
responses := []*http.Response{
{
Request: &http.Request{
URL: &url.URL{
Scheme: "https",
Host: "example.com",
Path: "some-file",
},
},
},
{
StatusCode: http.StatusPartialContent,
Body: ioutil.NopCloser(strings.NewReader("something")),
},
}
errors := []error{nil, nil}

httpClient.DoStub = func(req *http.Request) (*http.Response, error) {
count := httpClient.DoCallCount() - 1
return responses[count], errors[count]
}

ranger.BuildRangeReturns([]download.Range{download.NewRange(0, 15, http.Header{})}, nil)

downloader := download.Client{
Logger: &loggerfakes.FakeLogger{},
HTTPClient: httpClient,
Ranger: ranger,
Bar: bar,
Timeout: 5 * time.Millisecond,
}

closedFile, err := ioutil.TempFile("", "")
location, err := download.NewFileInfo(file)
Expect(err).NotTo(HaveOccurred())

err = closedFile.Close()
Expect(err).NotTo(HaveOccurred())

err = downloader.Get(closedFile, downloadLinkFetcher, GinkgoWriter)
Expect(err).To(MatchError(ContainSubstring("failed to read information from output file")))
err = downloader.Get(location, downloadLinkFetcher, GinkgoWriter)
Expect(err).To(MatchError("problem while waiting for chunks to download: failed during retryable request: during GET unexpected status code was returned: 500"))
})
})
})
Expand Down
9 changes: 4 additions & 5 deletions product_files.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"fmt"
"io"
"net/http"
"os"

"github.com/pivotal-cf/go-pivnet/download"
"github.com/pivotal-cf/go-pivnet/logger"
Expand Down Expand Up @@ -460,7 +459,7 @@ func (p ProductFilesService) RemoveFromFileGroup(
}

func (p ProductFilesService) DownloadForRelease(
location *os.File,
location *download.FileInfo,
productSlug string,
releaseID int,
productFileID int,
Expand All @@ -472,12 +471,12 @@ func (p ProductFilesService) DownloadForRelease(
productFileID,
)
if err != nil {
return err
return fmt.Errorf("GetForRelease: %s", err)
}

downloadLink, err := pf.DownloadLink()
if err != nil {
return err
return fmt.Errorf("DownloadLink: %s", err)
}

p.client.logger.Debug("Downloading file", logger.Data{"downloadLink": downloadLink})
Expand All @@ -492,7 +491,7 @@ func (p ProductFilesService) DownloadForRelease(
progressWriter,
)
if err != nil {
return err
return fmt.Errorf("Downloader.Get: %s", err)
}

return nil
Expand Down
30 changes: 23 additions & 7 deletions product_files_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"github.com/pivotal-cf/go-pivnet/download"
)

var _ = Describe("PivnetClient - product files", func() {
Expand Down Expand Up @@ -1124,8 +1125,8 @@ var _ = Describe("PivnetClient - product files", func() {
getStatusCode int
getResponse interface{}

downloadLinkResponseStatusCode int
cloudfrontDownloadPath string
downloadLinkResponseStatusCode int
cloudfrontDownloadPath string
)

BeforeEach(func() {
Expand Down Expand Up @@ -1231,8 +1232,11 @@ var _ = Describe("PivnetClient - product files", func() {
tmpFile, err := ioutil.TempFile("", "")
Expect(err).NotTo(HaveOccurred())

tmpLocation, err := download.NewFileInfo(tmpFile)
Expect(err).NotTo(HaveOccurred())

err = client.ProductFiles.DownloadForRelease(
tmpFile,
tmpLocation,
productSlug,
releaseID,
productFileID,
Expand All @@ -1259,8 +1263,11 @@ var _ = Describe("PivnetClient - product files", func() {
tmpFile, err := ioutil.TempFile("", "")
Expect(err).NotTo(HaveOccurred())

tmpLocation, err := download.NewFileInfo(tmpFile)
Expect(err).NotTo(HaveOccurred())

err = client.ProductFiles.DownloadForRelease(
tmpFile,
tmpLocation,
productSlug,
releaseID,
productFileID,
Expand All @@ -1279,8 +1286,11 @@ var _ = Describe("PivnetClient - product files", func() {
tmpFile, err := ioutil.TempFile("", "")
Expect(err).NotTo(HaveOccurred())

tmpLocation, err := download.NewFileInfo(tmpFile)
Expect(err).NotTo(HaveOccurred())

err = client.ProductFiles.DownloadForRelease(
tmpFile,
tmpLocation,
productSlug,
releaseID,
productFileID,
Expand Down Expand Up @@ -1341,8 +1351,11 @@ var _ = Describe("PivnetClient - product files", func() {
tmpFile, err := ioutil.TempFile("", "")
Expect(err).NotTo(HaveOccurred())

tmpLocation, err := download.NewFileInfo(tmpFile)
Expect(err).NotTo(HaveOccurred())

err = client.ProductFiles.DownloadForRelease(
tmpFile,
tmpLocation,
productSlug,
releaseID,
productFileID,
Expand All @@ -1366,8 +1379,11 @@ var _ = Describe("PivnetClient - product files", func() {
tmpFile, err := ioutil.TempFile("", "")
Expect(err).NotTo(HaveOccurred())

tmpLocation, err := download.NewFileInfo(tmpFile)
Expect(err).NotTo(HaveOccurred())

err = client.ProductFiles.DownloadForRelease(
tmpFile,
tmpLocation,
productSlug,
releaseID,
productFileID,
Expand Down

0 comments on commit 2172a68

Please sign in to comment.