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

fix newest-file selection for offline cache #216

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Strum355
Copy link

@Strum355 Strum355 commented Mar 1, 2024

(Issue discovered while using https://github.com/chainguard-dev/rules_apko)

Given the following keyrings:

- https://packages.sgdev.org/sourcegraph-melange-prod.rsa.pub
- https://packages.sgdev.org/chainguard/chainguard-enterprise.rsa.pub

the following directory structure is created:

cache_wolfi_base_apko/https%3A%2F%2Fpackages.sgdev.org%2F/
├─ chainguard/
│  ├─ chainguard-enterprise.rsa.pub
├─ sourcegraph-melange-prod.rsa.pub

This is fine when looking for the public key file for https://packages.sgdev.org/chainguard/chainguard-enterprise.rsa.pub, but may fail for https://packages.sgdev.org/sourcegraph-melange-prod.rsa.pub if the directory chainguard/ has a newer timestamp than the public key file (I was not able to reproduce this locally on darwin, but reliably on linux CI machines).

This PR addresses this by not selecting directories as possible "newest" dir entries.

@imjasonh
Copy link
Member

Hey sorry for missing this PR! 😭

In general the fix seems good, thanks for the thorough explanation of the problem. If there's any way you could add a test to demonstrate the behavior and the fix, that'd be helpful both for a.) confirming I understand the issue, and b.) ensuring future changes don't regress this fix unexpectedly.

Thanks again, and I think we'll be able to get this in once we have a simple test.

@Strum355
Copy link
Author

Strum355 commented Jun 4, 2024

Hey sorry for missing this PR! 😭

In general the fix seems good, thanks for the thorough explanation of the problem. If there's any way you could add a test to demonstrate the behavior and the fix, that'd be helpful both for a.) confirming I understand the issue, and b.) ensuring future changes don't regress this fix unexpectedly.

Thanks again, and I think we'll be able to get this in once we have a simple test.

Sorry for the long turnaround time. I've pushed a test case for this. LMK if this suffices

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.

2 participants