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

deleting empty files based on config #612

Merged
merged 7 commits into from
Nov 19, 2020
Merged

Conversation

jmbrown412
Copy link

This PR introduces logic to conditionally delete zero byte (empty) files that may exist on the remote server while a transfer is in place. We want to wait to delete these after a configurable amount of time.

@jmbrown412 jmbrown412 requested a review from adamdecaf as a code owner October 27, 2020 17:53
@jmbrown412 jmbrown412 linked an issue Oct 27, 2020 that may be closed by this pull request
@jmbrown412 jmbrown412 force-pushed the 602_zero_byte_file_cleanup branch from 7037153 to e88b5cb Compare October 27, 2020 17:58
Copy link
Member

@adamdecaf adamdecaf left a comment

Choose a reason for hiding this comment

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

Thanks for grabbing this!

pkg/config/odfi.go Outdated Show resolved Hide resolved
examples/config.yaml Outdated Show resolved Hide resolved
pkg/config/odfi.go Outdated Show resolved Hide resolved
pkg/transfers/inbound/cleanup.go Outdated Show resolved Hide resolved
pkg/transfers/inbound/cleanup.go Outdated Show resolved Hide resolved
@jmbrown412 jmbrown412 force-pushed the 602_zero_byte_file_cleanup branch from eed53f4 to faec143 Compare October 28, 2020 18:23
vxio
vxio previously approved these changes Oct 28, 2020
pkg/transfers/inbound/cleanup.go Outdated Show resolved Hide resolved
pkg/transfers/inbound/cleanup.go Outdated Show resolved Hide resolved
pkg/transfers/inbound/scheduler_test.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Oct 28, 2020

Codecov Report

Merging #612 into master will increase coverage by 0.21%.
The diff coverage is 74.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #612      +/-   ##
==========================================
+ Coverage   51.73%   51.94%   +0.21%     
==========================================
  Files          95       95              
  Lines        3236     3267      +31     
==========================================
+ Hits         1674     1697      +23     
- Misses       1211     1216       +5     
- Partials      351      354       +3     
Impacted Files Coverage Δ
pkg/config/odfi.go 30.00% <ø> (ø)
pkg/transfers/inbound/scheduler.go 44.18% <0.00%> (-2.16%) ⬇️
pkg/transfers/inbound/cleanup.go 76.00% <79.31%> (+4.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23a24c5...638e2da. Read the comment docs.

@adamdecaf
Copy link
Member

This looks really good @jmbrown412, nice job!

@adamdecaf adamdecaf force-pushed the 602_zero_byte_file_cleanup branch from a751aff to a1fe7ba Compare October 28, 2020 22:21
@jmbrown412 jmbrown412 force-pushed the 602_zero_byte_file_cleanup branch from a1fe7ba to 4ceebd7 Compare October 29, 2020 22:47
@jmbrown412 jmbrown412 force-pushed the 602_zero_byte_file_cleanup branch from 04e254a to a6c843d Compare October 30, 2020 15:42
@jmbrown412 jmbrown412 requested a review from adamdecaf October 30, 2020 16:00
InfernoJJ
InfernoJJ previously approved these changes Oct 30, 2020
pkg/transfers/inbound/cleanup.go Outdated Show resolved Hide resolved
pkg/transfers/inbound/cleanup.go Show resolved Hide resolved
pkg/transfers/inbound/cleanup.go Outdated Show resolved Hide resolved
vxio
vxio previously approved these changes Oct 30, 2020
pkg/transfers/inbound/cleanup.go Outdated Show resolved Hide resolved
vxio
vxio previously approved these changes Oct 30, 2020
pkg/transfers/inbound/cleanup.go Outdated Show resolved Hide resolved
pkg/transfers/inbound/cleanup.go Show resolved Hide resolved
@jmbrown412 jmbrown412 requested review from vxio and adamdecaf November 2, 2020 17:07
vxio
vxio previously approved these changes Nov 2, 2020
Comment on lines 46 to 51
if err := deleteEmptyFiles(logger, agent, dl.dir, agent.InboundPath(), now, after); err != nil {
el.Add(err)
}
if err := deleteEmptyFiles(logger, agent, dl.dir, agent.ReturnPath(), now, after); err != nil {
el.Add(err)
}
Copy link
Member

Choose a reason for hiding this comment

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

Up to you if you want to add this, but you could simplify to:

for _, path := range []string{agent.InboundPath(), agent.ReturnPath()} {
  if err := deleteEmptyFiles(logger, agent, dl.dir, path, now, after); err != nil {
		el.Add(err)
	}
}

Copy link
Author

Choose a reason for hiding this comment

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

I like this a lot better. Pushed

Copy link
Member

Choose a reason for hiding this comment

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

We should be consistent with Cleanup above. It's confusing when different coding styles are used within a file.

pkg/transfers/inbound/cleanup.go Outdated Show resolved Hide resolved
}

func (m mockedFS) Name() string {
panic("implement me")
Copy link
Contributor

@atonks2 atonks2 Nov 3, 2020

Choose a reason for hiding this comment

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

Should these just return "dummy" values, like return "name" instead of calling panic?

Copy link
Author

Choose a reason for hiding this comment

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

They totally should even though they aren't being used. I'll update these.

}
}

func Test_CleanupEmptyFiles_Not_Run_When_Config_Is_Zero(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these might be easier to read with fewer underscores.. Maybe something like TestCleanupEmptyFiles_notRunWhenConfigZero(t *testing.T)?

Copy link
Author

Choose a reason for hiding this comment

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

Agreed. I can clean these up here shortly.

func Test_CleanupEmptyFiles_InboundPath_Success(t *testing.T) {
agent := &upload.MockAgent{}

dir, _ := ioutil.TempDir("", "clenaup-testing")
Copy link
Contributor

Choose a reason for hiding this comment

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

All instances of clenaup-testing should be cleanup-testing

Copy link
Author

Choose a reason for hiding this comment

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

Totally. Good catch.

func Test_CleanupEmptyFiles_ReturnPath_Success(t *testing.T) {
agent := &upload.MockAgent{}

dir, _ := ioutil.TempDir("", "clenaup-testing")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be better to use t.TempDir() here instead because it cleans up after itself.

ioutil TempDir:

It is the caller's responsibility to remove the directory when no longer needed.

Compare to testing TempDir:

The directory is automatically removed by Cleanup when the test and all its subtests complete.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure on this one. Since the code being tested actually cleans up the files, I'm not sure if having it cleaned up automatically is what we want. Maybe? Need to look into it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It kind of cleans up. It cleans up the files, but leaves the directory. I think the testing package's version of the method makes more sense since it's purpose built for tests.

t.Error("expected error")
}

if agent.DeletedFile != "inbound/empty_file.ach" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This one's more of a nit, but I think tests cases and their output are easier to read when testify is used. As an example, this block could be replaced by assert.Equal(t, "inbound/empty_file.ach", agent.DeletedFile). If the test fails it will print out a nicely-formatted error message that lets you quickly see why the test failed.

Copy link
Author

Choose a reason for hiding this comment

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

Good to know. I'll try it out and see how it goes. If it makes reading test output better, more informative, nicely formatted then that sounds great.

@adamdecaf adamdecaf force-pushed the master branch 2 times, most recently from fb477cf to effcefb Compare November 8, 2020 23:29
@adamdecaf adamdecaf added this to the v0.10.0 milestone Nov 8, 2020
@adamdecaf adamdecaf merged commit 0e49860 into master Nov 19, 2020
@adamdecaf adamdecaf deleted the 602_zero_byte_file_cleanup branch November 19, 2020 18:40
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.

transfers/inbound: clean up zero-byte files
6 participants