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

feat: Column Resolvers #1301

Merged
merged 9 commits into from
Jul 28, 2022
Merged

feat: Column Resolvers #1301

merged 9 commits into from
Jul 28, 2022

Conversation

bbernays
Copy link
Contributor

Summary

2 Improvements:

  1. Using the funk.Get() method we can completely eliminate an entire class of issues where we try and access a nil pointers.
  2. Better able to autogenerate the resolver as we don't have to check each value to see if it is nil

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 July 26, 2022 11:27
@bbernays bbernays requested review from disq and removed request for a team July 26, 2022 11:28
yevgenypats
yevgenypats suggested changes Jul 26, 2022
View reviewed changes
Copy link
Member

@yevgenypats yevgenypats left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'd love to have a sync discussion but in general Im not in favour of loosing more type safety. In that case nil-deref error will stop being sent out and our permutation/mock tests also wont catch those any more.

yevgenypats
yevgenypats approved these changes Jul 26, 2022
View reviewed changes
Copy link
Member

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

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

Revert my first comment - looks like mock tests should catch accessing wrong fields. Maybe can be improved even further with reflection/generic.

@bbernays
Copy link
Contributor Author

This PR is now blocked by this pr

@candiduslynx
Copy link
Contributor

I suppose the sdk needs to be updated as well

roneli
roneli reviewed Jul 27, 2022
View reviewed changes
Copy link
Contributor

@roneli roneli left a comment

Choose a reason for hiding this comment

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

Looks great, some rogue fields and linting issues, but I like this improvement

disq
disq approved these changes Jul 28, 2022
View reviewed changes
Copy link
Member

@disq disq left a comment

Choose a reason for hiding this comment

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

Could update the sdk reference to be github.com/cloudquery/cq-provider-sdk@a5db808 a5db808 to get rid of the linting errors... or just wait until a new SDK is tagged ofc

@kodiakhq kodiakhq bot merged commit 9b2dbed into cloudquery:main Jul 28, 2022
@cq-bot cq-bot mentioned this pull request Jul 28, 2022
Merged
@bbernays bbernays deleted the ecs-improvements branch July 28, 2022 22:05
kodiakhq bot pushed a commit that referenced this pull request Aug 2, 2022
@cq-bot
chore(main): Release v0.13.4 (#1339)
d53de22 
🤖 I have created a release *beep* *boop*
---


## [0.13.4](v0.13.3...v0.13.4) (2022-08-02)


### Features

* Add Kinesis Data Stream support ([#1348](#1348)) ([767bfab](767bfab))
* Add Tags for ECR Repo ([#1369](#1369)) ([3b31598](3b31598))
* Added glue databases and tables ([#1345](#1345)) ([0284a37](0284a37))
* Added glue jobs ([#1352](#1352)) ([562a6b3](562a6b3))
* Column Resolvers    ([#1301](#1301)) ([9b2dbed](9b2dbed))


### Bug Fixes

* **deps:** Update module github.com/cloudquery/cq-gen to v0.0.7 ([#1362](#1362)) ([3060854](3060854))
* **deps:** Update module github.com/hashicorp/go-hclog to v1.2.2 ([#1350](#1350)) ([82ec301](82ec301))
* Update endpoints ([#1347](#1347)) ([3191f3e](3191f3e))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
erezrokah pushed a commit to cloudquery/cloudquery that referenced this pull request Aug 14, 2022
@cq-bot
chore(main): Release v0.13.4 (#1339)
6d2c426 
🤖 I have created a release *beep* *boop*
---


## [0.13.4](cloudquery/cq-provider-aws@v0.13.3...v0.13.4) (2022-08-02)


### Features

* Add Kinesis Data Stream support ([#1348](cloudquery/cq-provider-aws#1348)) ([767bfab](cloudquery/cq-provider-aws@767bfab))
* Add Tags for ECR Repo ([#1369](cloudquery/cq-provider-aws#1369)) ([3b31598](cloudquery/cq-provider-aws@3b31598))
* Added glue databases and tables ([#1345](cloudquery/cq-provider-aws#1345)) ([0284a37](cloudquery/cq-provider-aws@0284a37))
* Added glue jobs ([#1352](cloudquery/cq-provider-aws#1352)) ([562a6b3](cloudquery/cq-provider-aws@562a6b3))
* Column Resolvers    ([#1301](cloudquery/cq-provider-aws#1301)) ([9b2dbed](cloudquery/cq-provider-aws@9b2dbed))


### Bug Fixes

* **deps:** Update module github.com/cloudquery/cq-gen to v0.0.7 ([#1362](cloudquery/cq-provider-aws#1362)) ([3060854](cloudquery/cq-provider-aws@3060854))
* **deps:** Update module github.com/hashicorp/go-hclog to v1.2.2 ([#1350](cloudquery/cq-provider-aws#1350)) ([82ec301](cloudquery/cq-provider-aws@82ec301))
* Update endpoints ([#1347](cloudquery/cq-provider-aws#1347)) ([3191f3e](cloudquery/cq-provider-aws@3191f3e))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@roneli roneli roneli left review comments

@disq disq disq approved these changes

@candiduslynx candiduslynx candiduslynx approved these changes

@yevgenypats yevgenypats yevgenypats approved these changes

Assignees
No one assigned
Labels
automerge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@bbernays @candiduslynx @disq @yevgenypats @roneli