Skip to content
This repository has been archived by the owner on Aug 16, 2022. It is now read-only.

feat: Simplify Resources #1385

Merged
merged 18 commits into from
Aug 7, 2022
Merged

Conversation

bbernays
Copy link
Contributor

@bbernays bbernays commented Aug 4, 2022

Summary

More simplification of resource resolvers


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

  • Read the contribution guidelines πŸ§‘β€πŸŽ“
  • Run go fmt to format your code πŸ–Š
  • Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
  • Update or add tests. Learn more about testing here πŸ§ͺ
  • Update the docs by running go run ./docs/docs.go and committing the changes πŸ“ƒ
  • If adding a new resource, add relevant Terraform files in a separate PR πŸ“‚
  • Ensure the status checks below are successful βœ…

@bbernays bbernays requested a review from a team as a code owner August 4, 2022 19:41
@bbernays bbernays requested review from shimonp21 and hermanschaaf and removed request for a team August 4, 2022 19:41
client/resolvers.go Outdated Show resolved Hide resolved
@hermanschaaf
Copy link
Member

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

@hermanschaaf
Copy link
Member

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
Copy link
Contributor Author

bbernays commented Aug 5, 2022

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

func resolveWafWebACLLoggingConfigurationRedactedFields(ctx context.Context, meta schema.ClientMeta, resource *schema.Resource, c schema.Column) error {
if conf := resource.Item.(*types.LoggingConfiguration); conf != nil {
out, err := json.Marshal(conf.RedactedFields)
if err != nil {
return diag.WrapError(err)
}
return diag.WrapError(resource.Set(c.Name, out))
}
return nil
}

@bbernays
Copy link
Contributor Author

bbernays commented Aug 5, 2022

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

@bbernays bbernays changed the title feat: PathJson resolver feat: Simplify Resources Aug 5, 2022
@amanenk
Copy link
Contributor

amanenk commented Aug 5, 2022

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.

@bbernays bbernays requested a review from erezrokah August 7, 2022 15:22
Copy link
Contributor

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

🧹

@bbernays bbernays merged commit 1f7eab8 into cloudquery:main Aug 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants