Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

CopyWalker: truncate file when writing #56

Merged
merged 2 commits into from Feb 27, 2020
Merged

Conversation

invidian
Copy link
Member

To avoid leaving old content untouched when updating files, which may
happen if new content is smaller than the old one.

Closes #40

Signed-off-by: Mateusz Gozdek mateusz@kinvolk.io

Copy link
Contributor

@iaguis iaguis left a comment

Choose a reason for hiding this comment

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

Some comments but looks good in general.

Great catch!

pkg/util/walkers/copying.go Outdated Show resolved Hide resolved
pkg/util/walkers/copying_test.go Outdated Show resolved Hide resolved
pkg/util/walkers/copying_test.go Outdated Show resolved Hide resolved
pkg/util/walkers/copying.go Outdated Show resolved Hide resolved
pkg/util/walkers/copying_test.go Show resolved Hide resolved
iaguis
iaguis previously approved these changes Feb 27, 2020
Copy link
Contributor

@iaguis iaguis left a comment

Choose a reason for hiding this comment

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

A couple of nits but LGTM.

pkg/util/walkers/copying.go Outdated Show resolved Hide resolved
defer f.Close()

if _, err := io.Copy(f, r); err != nil {
return fmt.Errorf("failed writing to file %s: %v", p, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("failed writing to file %s: %v", p, err)
return fmt.Errorf("failed writing to file %s: %w", p, err)

@iaguis
Copy link
Contributor

iaguis commented Feb 27, 2020

I'll modify the PR to use %w and create an issue to go through our uses of fmt.Errorf and errors.Wrap* and update them to the new convention: #59

So at least some changes to this function can be easily tested.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@iaguis iaguis force-pushed the invidian/fix-copying-walker branch 4 times, most recently from b8231dd to 4aab93c Compare February 27, 2020 13:59
To avoid leaving old content untouched when updating files, which may
happen if new content is smaller than the old one.

Closes #40

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
Copy link
Contributor

@iaguis iaguis left a comment

Choose a reason for hiding this comment

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

lgtm

@iaguis iaguis merged commit 5004c76 into master Feb 27, 2020
@iaguis iaguis deleted the invidian/fix-copying-walker branch February 27, 2020 22:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Writing assets to disk sometimes produces bad Terraform code
2 participants