cloudquery / plugin-sdk

CloudQuery Go SDK for source and destination plugins
Mozilla Public License 2.0
21 stars 23 forks source link

Spec `Path` and `Registry` should not be a part of the JSON configuration #176

Closed erezrokah closed 1 year ago

erezrokah commented 1 year ago

I think we can just expose Name with the following use cases:

# GitHub `cloudquery/azure`
name: azure

# GitHub `org/cq-plugin-datadog`
name: org/cq-plugin-datadog

# Local (notice value starts with `.`)
name: ./path-to-binary

# gRPC (notice value starts with http://)
name: http://localhost:7777

We should also consider dropping Local or gRPC as they serve the same purpose.

Internally we have still parse name into Path and Registry but it doesn't need to be a part of the configuration file

yevgenypats commented 1 year ago

We've tried that in the past it get's complicated with a lot of edge cases very quickly.

name is an alias registry: can be grpc, local and github (might have others in the future) parsing generic URI is a real rabbit hole so better to use config that is not allowing parsing errors.

Also you can have multiple azure plugins with different configuration so you need to be able to address them azure, azure1 (because we will use it in the logs) but they will have the same path.

erezrokah commented 1 year ago

registry: can be grpc, local and github (might have others in the future) parsing generic URI is a real rabbit hole so better to use config that is not allowing parsing errors.

Also you can have multiple azure plugins with different configuration so you need to be able to address them azure, azure1 (because we will use it in the logs) but they will have the same path.

So maybe only keep Path - there's a clear 1-1 mapping between the path and the registry. We can always add registry later on, and I'm assuming most users won't need to configure it

yevgenypats commented 1 year ago

we have to keep it other it wont work: both it also makes the code much easier without going into understanding from a string weather it's a local file or s3 file or github or grpc. It is set by default to github so the user shouldn't be aware of this at all. it's just for the developers and we really need that. because let's say we introduce:

http://localhost:7777

Should we download from there? do we need to parse an extension? should we connect over http (if we introduce http). There is no way of knowing just by parsing a URI (or even if there is right now it will break very soon and the reason I introduced it was because it was already with the parsing and it didn't work becuase in the past users asked for us to be able to support downloading plugins from https and s3 which wont work if we dont have the registry option)

erezrokah commented 1 year ago

I think there are solutions to all those use cases (e.g. try to connect via a protocol, see if it matches, if not use next one), but it's probably too soon to optimize for those. I'll close the issue as we can always decide to remove those options if we'd like

yevgenypats commented 1 year ago

I think there are solutions to all those use cases (e.g. try to connect via a protocol, see if it matches, if not use next one), but it's probably too soon to optimize for those. I'll close the issue as we can always decide to remove those options if we'd like

Agree to soon to optimize but actually there isn't a solution and I can show an example:

Let's say we want to add the following support:

There is no way of telling the plugin manager to distinguish between http://something.something/somefile and http://something.something/somefile (because it's the same URL) but we actually want a completly different behaviour. for 1) we just want to connect to it. for 2) we want to download it.

yevgenypats commented 1 year ago

I can understand the confusion but seems to me more of a naming issue rather then an incorrect implementation (as there is no other technically feasible way to achieve that). I agree registry is confusing so maybe we can do something like

source instead and then have:

future:

So we can add various behaviours and control it rather then parsing URI and inventing new uris which will be clashing with others and so on connect-only+grpc://

erezrokah commented 1 year ago

There is no way of telling the plugin manager to distinguish between http://something.something/somefile and http://something.something/somefile (because it's the same URL) but we actually want a completly different behaviour. for 1) we just want to connect to it. for 2) we want to download it.

I think we can try a gRPC connection then if it succeeds, assume a gRPC plugin, if it fails try to download from http:, etc. i.e duck typing, meaning if it behaves like a gRPC plugin it is one, and if it behaves like an HTTP endpoint with a zip to download we can assume it's a download URL.

erezrokah commented 1 year ago

I know it's kind of similar to what we had with go downloader library, but I think that as long as we don't have overlaps this should work. Once we do we could add more configuration. But again, too soon to optimize

yevgenypats commented 1 year ago

Im really against trying/retrying something just to avoid having another flag which will define what the user wants. it creates hard to debug errors messages and spagetti code with fallbacks that increases with every new option.

The only thing we can do imo is change from registry -> source (or other name).

erezrokah commented 1 year ago

The only thing we can do imo is change from registry -> source (or other name).

I would not use source as we already have use it 🙃

Maybe just remove registry from the docs and only keep in in the developing a plugin guide?

yevgenypats commented 1 year ago

Let's keep it as is then as users asked for local mode number of times as well :) Closing then