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

test(bigtable): adds test proxy for Bigtable test suite #6568

Merged
merged 64 commits into from Oct 3, 2022
Merged

Conversation

telpirion
Copy link
Contributor

For use with the Bigtable test suite.

@telpirion telpirion added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 25, 2022
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: bigtable Issues related to the Bigtable API. labels Aug 25, 2022
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Aug 31, 2022
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Sep 9, 2022
@enocom enocom self-assigned this Sep 29, 2022
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels Sep 30, 2022
Copy link
Member

@enocom enocom left a comment

Choose a reason for hiding this comment

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

Some minor comments and then I think this is ready to merge.

l := alf.ApplyLabelTransformer
al := bigtable.LabelFilter(l)
f = &al
default:
Copy link
Member

Choose a reason for hiding this comment

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

Since you're returning f below which would be nil if any of these case statements don't match, you can remove this branch OR just put a return in each case statement and remove the return f below. I personally find returning out of case statements easier to read when it gets this long.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compiler doesn't like the missing return statement, so I just deleted the default branch.

bigtable/internal/testproxy/proxy.go Show resolved Hide resolved

return nil, nil
}
return creds, nil
Copy link
Member

Choose a reason for hiding this comment

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

You might consider returning out of the case statements above just because this switch statement is a little long. Up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compiler doesn't like it if I remove the final return statement.

bigtable/internal/testproxy/proxy.go Outdated Show resolved Hide resolved
bigtable/internal/testproxy/proxy_test.go Outdated Show resolved Hide resolved
bigtable/internal/testproxy/proxy_test.go Outdated Show resolved Hide resolved
bigtable/internal/testproxy/proxy_test.go Show resolved Hide resolved
bigtable/internal/testproxy/proxy_test.go Outdated Show resolved Hide resolved
@telpirion telpirion added the automerge Merge the pull request once unit tests and other checks pass. label Oct 3, 2022
@telpirion telpirion requested a review from enocom October 3, 2022 19:20

switch mut := mpb.Mutation; mut.(type) {
case *btpb.Mutation_DeleteFromColumn_:
del := mut.(*btpb.Mutation_DeleteFromColumn_)
Copy link
Member

Choose a reason for hiding this comment

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

Change the switch statement above to switch mut := mpb.Mutation.(type) { and you'll be able to remove these type assertions.

// Filter object.
func filterFromProto(rfPb *btpb.RowFilter) *bigtable.Filter {
var f *bigtable.Filter
switch fpb := rfPb.Filter; fpb.(type) {
Copy link
Member

Choose a reason for hiding this comment

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

switch fpb := rfPb.Filter.(type) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


t.Log("testproxy test: client created successfully in test proxy")

_, err = (*client).RemoveClient(ctx, &pb.RemoveClientRequest{
Copy link
Member

Choose a reason for hiding this comment

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

client.RemoveClient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

InstanceId: "instance",
}

_, err := (*client).CreateClient(ctx, req)
Copy link
Member

Choose a reason for hiding this comment

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

client.CreateClient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@telpirion telpirion merged commit 373d2fc into main Oct 3, 2022
@telpirion telpirion deleted the bigtable-proxy branch October 3, 2022 22:11
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Oct 3, 2022
gcf-merge-on-green bot pushed a commit that referenced this pull request Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API. size: l Pull request size is large. stale: old Pull request is old and needs attention.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants