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.

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

Small changes to prep for caching daemon #90

Merged
merged 4 commits into from
May 8, 2024
Merged

Small changes to prep for caching daemon #90

merged 4 commits into from
May 8, 2024

Conversation

airhorns
Copy link
Contributor

@airhorns airhorns commented May 5, 2024

  • Clarify contributing instructions (and move to their own file)
  • Add the count of written objects to the return value of GetCache
  • Ensure packObjects test helper produces deterministic hashes by iterating in sorted order

@airhorns airhorns changed the title Clarify contributing instructions (and move to their own file) Small changes to prep for caching daemon May 5, 2024
@airhorns airhorns marked this pull request as ready for review May 5, 2024 17:03
@airhorns airhorns requested a review from angelini May 5, 2024 17:03
make build
```

### Run migrations
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this section supposed to have make migrate instructions?

@@ -49,7 +49,7 @@ func TestClientGetCache(t *testing.T) {
tmpCacheDir, err := os.MkdirTemp("", "dl_cache_test_tmp")
require.NoError(t, err)

version, err := c.GetCache(tc.Context(), tmpCacheDir)
version, _, err := c.GetCache(tc.Context(), tmpCacheDir)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a bad idea to add a couple assertions on this value to make sure it's returning what we expect.

keys = append(keys, key)
}

sort.Strings(keys)
Copy link
Contributor

Choose a reason for hiding this comment

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

Damn, this was kind of a consequence of two things:

  • Reproducing packObjects as a test util instead of using the same function we do in prod
  • The fact that the prod version of packObjects expects a sorted stream and we use a map for convenience in tests

The prod sorting happens here: https://github.com/gadget-inc/dateilager/blob/main/internal/db/tar.go#L233

@angelini
Copy link
Contributor

angelini commented May 8, 2024

FYI, I'm going to push commits to this branch

@angelini angelini changed the base branch from harry/multi-stage-docker to main May 8, 2024 10:46
@angelini angelini merged commit 50e15c9 into main May 8, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants