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

Jump to bottom

Added cache memory exceed check #24

Merged
merged 5 commits into from
Jul 20, 2023
Merged

Added cache memory exceed check #24

merged 5 commits into from
Jul 20, 2023

Conversation

AvineshTripathi
Copy link
Contributor

Description

What type of PR is this? (check all applicable)

  • 🍕 Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 🎨 Style
  • 🧑‍💻 Code Refactor
  • 🔥 Performance Improvements
  • ✅ Test
  • 🤖 Build
  • 🔁 CI
  • 📦 Chore (Release)
  • ⏩ Revert

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help

Added to documentation?

  • 📜 README.md
  • 📓 docs.opensauced.pizza
  • 🍕 dev.to/opensauced
  • 📕 storybook
  • 🙅 no documentation needed

fixes #22

jpmcb
jpmcb requested changes Jul 20, 2023
View reviewed changes
Copy link
Member

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

👏🏼 👏🏼 🕺🏼

Very nice! Thanks for this contribution!

Nit: remove the TODO comments on line 54 with this change

pkg/cache/lrucache.go Outdated Show resolved Hide resolved
pkg/cache/lrucache.go Outdated Show resolved Hide resolved
pkg/cache/lrucache.go Outdated Show resolved Hide resolved
jpmcb
jpmcb requested changes Jul 20, 2023
View reviewed changes
Copy link
Member

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

Nit: file isn't formatted. Run go fmt ./... to resolved the "goimports" linting error.

pkg/cache/lrucache.go Outdated Show resolved Hide resolved
jpmcb
jpmcb requested changes Jul 20, 2023
View reviewed changes
Copy link
Member

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

Look at that! The failing test tripped this check correctly:

    gitrepofilepath_test.go:25: unexpected err: minimum free disk space: 100 exceeds actual available disk space: 23865319424
--- FAIL: TestOpenAndFetch (0.00s)
    --- FAIL: TestOpenAndFetch/Puts_repos_into_cache_in_sequential_order (0.00s)

Which is coming from:

pizza/pkg/cache/gitrepofilepath_test.go

Line 23 in 51ed9ca

c, err := NewGitRepoLRUCache(tt.cacheDir, 100)

This should be 1 instead of 100 since the github runners typically only have like 20gbs of disk on them.

pkg/cache/lrucache.go Outdated Show resolved Hide resolved
jpmcb
jpmcb approved these changes Jul 20, 2023
View reviewed changes
Copy link
Member

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

Looks great!!!!! 👏🏼 👏🏼 very well done!!

brandonroberts
brandonroberts approved these changes Jul 20, 2023
View reviewed changes
Copy link

@brandonroberts brandonroberts left a comment

Choose a reason for hiding this comment

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

Nice!

@jpmcb jpmcb merged commit 03f45fb into open-sauced:main Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpmcb jpmcb jpmcb approved these changes

@brandonroberts brandonroberts brandonroberts 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.

Feature: NewGitRepoLRUCache should error if disk space <= configured minimum free disk space for cache
3 participants
@AvineshTripathi @brandonroberts @jpmcb