cloudquery / cq-provider-aws

CloudQuery Provider for AWS
https://cloudquery.io
Mozilla Public License 2.0
29 stars 40 forks source link

feat: Simplify Resources #1385

Closed bbernays closed 2 years ago

bbernays commented 2 years ago

Summary

More simplification of resource resolvers


Use the following steps to ensure your PR is ready to be reviewed

hermanschaaf commented 2 years ago

@bbernays it looks like the PR contains a couple of commits from another branch and will need to be rebased

hermanschaaf commented 2 years ago

Maybe I'm missing something, but I think this is already possible. When using cq-gen, you can set the column type to be JSON. AFAICT It then uses PathResolver and does the JSON conversion for you.

Here is a test case for this in cq-gen:

So I'm not sure where PathJSONResolver would be needed?

bbernays commented 2 years ago

I fully agree that if PathResolver can do the marshaling then this resolver is 100% not necessary. @roneli, @amanenk and @irmatov Can speak to why we have so many resolvers like this that as @hermanschaaf points out could possibly be handled by the PathResolver

https://github.com/cloudquery/cq-provider-aws/blob/01b56da6d86a41804ea7ddcce29946840041f9fc/resources/services/waf/web_acls.go#L254-L263

bbernays commented 2 years ago

As @hermanschaaf has pointed out, this resolver is not necessary as our internal implementation for json columns already calls json.marshal https://github.com/cloudquery/cq-provider-sdk/blob/76ac8db01622b520bbacfe0272ab38233eb1abdf/provider/schema/dialect.go#L242-L251

amanenk commented 2 years ago

It is totally fine to regenerate all the resources and use PathResolver. Before some sdk implementations not all the structures had automatically marshaled to json.