feat: Add function to list tables that match a source spec#440
feat: Add function to list tables that match a source spec#440kodiakhq[bot] merged 10 commits intomainfrom
Conversation
yevgenypats
left a comment
There was a problem hiding this comment.
Will it make sense to introduce another method for that? Otherwise the client can think this is supported while it doesn't and if it will be a new method it can distinguish that. Not sure if this is needed as it can also work that way. but just bringing this up for discussion.
|
@yevgenypats Hmm yeah, good point. We don't need to know right now, but I can see it potentially being important/useful to avoid bugs in the future. I'm fine with adding an extra method if you're happy with that too. |
⏱️ Benchmark resultsComparing with c8c824d
|
…k into add-tables-for-spec
clients/source.go
Outdated
| "google.golang.org/grpc/status" | ||
| ) | ||
|
|
||
| var ErrMethodNotSupported = errors.New("method not supported") |
There was a problem hiding this comment.
I added this error so that callers of this client library don't have to unwrap the returned error and figure out from grpc status codes whether it was supported or not. They can now check whether this error was returned to know if a method was not supported.
There was a problem hiding this comment.
I don't think we should do that because but rather we need to use the grpc code directly. The CLI can have a helper function that check if a grpc is unimplemented and return some error message but the client shouldn't introduce a new error.
There was a problem hiding this comment.
Maybe I misunderstand the purpose of the client library, but my thinking is that it should abstract away the gRPC calls so that users of this library don't have to concern themselves with the underlying gRPC layer? (Otherwise this has a leaky abstraction)
There was a problem hiding this comment.
Oh yeah, it's probably also worth mentioning that the gRPC status codes don't support errors.As, otherwise it would have been okay to wrap the error before returning it: grpc/grpc-go#2934.
But as things stand, it's pretty tricky for callers of the client library to discover when it's not supported in a reliable way. Probably the best would be to look for a specific string in the error message.
There was a problem hiding this comment.
I see. ok I guess it's a nit. not sure right now what would be the best solution so let's skip it. mostly the thing that worries me that it is now inconsistent because then we need to put it in all calls.
|
@yevgenypats I've added the method, and updated the corresponding PR here cloudquery/cloudquery#5098 to make sure it all works together. Here's an updated example using the new code: And if a new CLI is run against an old version of the AWS plugin: Please take another look 🙇 |
yevgenypats
left a comment
There was a problem hiding this comment.
Looks good. One comment on error handling and I think one more "e2e" test is missing here https://github.com/cloudquery/plugin-sdk/blob/main/serve/source_test.go
|
@yevgenypats Thanks for the review! Updated with the following changes:
|
🤖 I have created a release *beep* *boop* --- ## [1.10.0](v1.9.0...v1.10.0) (2022-11-29) ### Features * Add function to list tables that match a source spec ([#440](#440)) ([a8f3690](a8f3690)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Depends on this plugin-sdk change: cloudquery/plugin-sdk#440 Fixes #4770
This adds a new
GetTablesForSpecfunction to theSourceClientthat filters the list of tables to the ones selected by the given Source Spec. It is fully backwards-compatible, and I decided to keep the changeset small by adding an optional parameter to the existingGetTablesproto rpc.This will be used to implement cloudquery/cloudquery#4715