Skip to content
This repository was archived by the owner on Mar 27, 2024. It is now read-only.

add layer caching #118

Merged
merged 6 commits into from
Oct 20, 2017
Merged

add layer caching #118

merged 6 commits into from
Oct 20, 2017

Conversation

nkubala
Copy link
Contributor

@nkubala nkubala commented Oct 16, 2017

slightly modified version of #102

adds support for disabling cache through CLI, and provides interface for adding other cache implementations later on (i.e. in-memory caching)

cc @dlorenc

@nkubala
Copy link
Contributor Author

nkubala commented Oct 16, 2017 •
edited
Loading

integration tests run over 3x faster 👍

➜  container-diff git:(cache) make test integration
go test -v -tags integration github.com/GoogleCloudPlatform/container-diff/tests -timeout 20m
    --- PASS: TestDiffAndAnalysis/pip_analysis (62.17s)
    --- PASS: TestDiffAndAnalysis/history_differ (64.61s)
    --- PASS: TestDiffAndAnalysis/multi_differ_local (64.69s)
    --- PASS: TestDiffAndAnalysis/apt_differ (67.13s)
    --- PASS: TestDiffAndAnalysis/file_differ (70.02s)
    --- PASS: TestDiffAndAnalysis/node_analysis (29.32s)
    --- PASS: TestDiffAndAnalysis/file_sorted_analysis (25.47s)
    --- PASS: TestDiffAndAnalysis/apt_analysis (25.51s)
PASS
ok  	github.com/GoogleCloudPlatform/container-diff/tests	81.372s

➜  container-diff git:(master) make test integration
go test -v -tags integration github.com/GoogleCloudPlatform/container-diff/tests -timeout 20m
    --- PASS: TestDiffAndAnalysis/multi_differ (194.85s)
    --- PASS: TestDiffAndAnalysis/file_differ (206.31s)
    --- PASS: TestDiffAndAnalysis/history_differ (37.23s)
    --- PASS: TestDiffAndAnalysis/apt_analysis (239.63s)
    --- PASS: TestDiffAndAnalysis/node_differ (255.11s)
    --- PASS: TestDiffAndAnalysis/apt_differ (84.32s)
    --- PASS: TestDiffAndAnalysis/multi_differ_local (76.16s)
    --- PASS: TestDiffAndAnalysis/pip_analysis (261.47s)
PASS
ok  	github.com/GoogleCloudPlatform/container-diff/tests	262.901s

dlorenc
dlorenc reviewed Oct 16, 2017
View reviewed changes
Copy link
Contributor

@dlorenc dlorenc left a comment

Choose a reason for hiding this comment

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

Nice!

pkg/cache/file_cache.go
@@ -0,0 +1,62 @@
/*
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 add some unit tests for this file?

dlorenc
dlorenc reviewed Oct 19, 2017
View reviewed changes
util/file_cache_test.go Outdated
}
}

func TestMain(m *testing.M) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't need a TestMain function, it's a bit of an anti-pattern now with subtests.

dlorenc
dlorenc reviewed Oct 19, 2017
View reviewed changes
util/file_cache_test.go Outdated
defer os.RemoveAll(cacheDir)
c, err := cache.NewFileCache(cacheDir)
if err != nil {
fmt.Printf(err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be a t.Fatal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops yes meant to change to that, this was originally in TestMain so we didn't have a testing.T

@dlorenc
Copy link
Contributor

dlorenc commented Oct 20, 2017

Looks like you need gazelle then you're done.

@nkubala nkubala merged commit 4ceba80 into GoogleContainerTools:master Oct 20, 2017
@nkubala nkubala deleted the cache branch October 20, 2017 18:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dlorenc dlorenc dlorenc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nkubala @dlorenc