Skip to content

feat(storage): add StartOffset and EndOffset to Query#2563

Merged
AlisskaPie merged 13 commits intogoogleapis:masterfrom
MaxxleLLC:add_offsets_to_query
Jul 22, 2020
Merged

feat(storage): add StartOffset and EndOffset to Query#2563
AlisskaPie merged 13 commits intogoogleapis:masterfrom
MaxxleLLC:add_offsets_to_query

Conversation

@AlisskaPie
Copy link
Copy Markdown
Contributor

@AlisskaPie AlisskaPie commented Jul 8, 2020

Closes #2556

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 8, 2020
@AlisskaPie AlisskaPie marked this pull request as ready for review July 8, 2020 16:55
@AlisskaPie AlisskaPie requested a review from tritone as a code owner July 8, 2020 16:55
@AlisskaPie
Copy link
Copy Markdown
Contributor Author

@tritone Do we need to add a test here? If so, where it will be better to add this test?
I've tested it in golang-samples repo, and it works correctly.

Copy link
Copy Markdown
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

I think we'd definitely want a test for this; I need to look at existing tests for ObjectIterator to see what makes sense.

@frankyn can you confirm that we'd want to expose this feature through the client library? Looks like it was a request from an external user.

@frankyn
Copy link
Copy Markdown
Contributor

frankyn commented Jul 9, 2020

Hi @tritone, yes these fields are okay to surface as they're published in public documentation: https://cloud.google.com/storage/docs/json_api/v1/objects/list

Copy link
Copy Markdown
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

I think an integration test might make sense for this feature-- something like

func testObjectIterator(t *testing.T, bkt *BucketHandle, objects []string) {
and the tests below that. @AlisskaPie could you create this?

@AlisskaPie
Copy link
Copy Markdown
Contributor Author

I think an integration test might make sense for this feature-- something like

google-cloud-go/storage/integration_test.go

Line 1055 in 2a870c2

func testObjectIterator(t *testing.T, bkt *BucketHandle, objects []string) {
and the tests below that. @AlisskaPie could you create this?

Yes, I'll do this.
Thank you for the detailed answer!

@AlisskaPie AlisskaPie marked this pull request as draft July 14, 2020 07:53
@AlisskaPie AlisskaPie marked this pull request as ready for review July 15, 2020 13:42
@AlisskaPie AlisskaPie requested a review from tritone July 15, 2020 13:48
Copy link
Copy Markdown
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Looks good-- a couple comments re: testing but should be ready to go once these are fixed.

- new function testObjectIteratorWithOffset
- leave it in the AllSelectedAttrs
- remove it in the SelectedAttrs
Copy link
Copy Markdown
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@tritone tritone added the automerge Merge the pull request once unit tests and other checks pass. label Jul 22, 2020
@AlisskaPie AlisskaPie merged commit bb1907d into googleapis:master Jul 22, 2020
@AlisskaPie AlisskaPie deleted the add_offsets_to_query branch July 23, 2020 10:00
tritone added a commit to tritone/google-cloud-go that referenced this pull request Aug 25, 2020
* add StartOffset and EndOffset to Query

* fix comments

* add testObjectIteratorOffset, test for StartOffset

* cosmetic

* move to testObjectIterator

* test for EndOffset

* small fixes

* added offsets to testObjectsIterateSelectedAttrs

* added offset to testObjectsIterateAllSelectedAttrs

* review changes

- new function testObjectIteratorWithOffset
- leave it in the AllSelectedAttrs
- remove it in the SelectedAttrs

Co-authored-by: Chris Cotter <cjcotter@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge Merge the pull request once unit tests and other checks pass. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

storage: support startOffset query during object listing

4 participants