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

add regular seek #29

Closed
wants to merge 1 commit into from

Conversation

thaJeztah
Copy link
Contributor

I'm not the original author of this patch, but trying to see if this patch can be upstreamed. BuildKit (https://github.com/moby/buildkit) has been using a fork of this project with this patch applied for the last 3 years; depending on a fork is not "ideal", so I'm opening this pull request to discuss the option of upstreaming this patch (also see the discussion on tonistiigi#1).

I must admit that I don't have a lot of background around the purpose of this patch, so pinging @tonistiigi to provide more details if needed (also if tests are needed (happy to give you write access to my branch, or apply additional patches))

Signed-off-by: Tonis Tiigi <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@hashicorp-cla
Copy link

hashicorp-cla commented May 13, 2020

CLA assistant check
All committers have signed the CLA.

@thaJeztah
Copy link
Contributor Author

ping @banks ptal

/cc @tonistiigi @AkihiroSuda @hinsun (BuildKit maintainers)

@thaJeztah
Copy link
Contributor Author

Hmm.. looks like @tonistiigi has to sign the CLA as well

@banks
Copy link
Member

banks commented May 13, 2020

Hi @thaJeztah thanks for the background. I'm glad to hear this has been helping out in BuildKit.

I'd guess the intention of this patch from reading it was to allow for "range scans" i.e: seek to a prefix and then return a Seeker that will iterate over each key greater than or equal to that prefix.

The good news (kinda) is that I recently added equivalent functionality to this library. See #24. The naming and API are slightly different but it's already here and as far as I can see does the same thing (and has tests!).

Would you be able to confirm that it meets your needs as I'd rather not merge another interface to do the same thing?

@thaJeztah
Copy link
Contributor Author

That sounds great! I'll let the BuildKit maintainers have a look (as I'm really not familiar with this library or its use myself); if the new feature would work, we can close this PR (and I'm a happy person 🎉)

@thaJeztah
Copy link
Contributor Author

Closing in favour of #39 (thanks!)

@thaJeztah thaJeztah closed this Jun 29, 2021
@thaJeztah thaJeztah deleted the upstream_regular_seek branch June 29, 2021 20:40
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.

4 participants