Skip to content

Conversation

@erezrokah
Copy link
Member

@erezrokah erezrokah commented Sep 20, 2022

Summary

Fixes #114


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 πŸ§ͺ
  • Ensure the status checks below are successful βœ…

schema/table.go Outdated
}
}

totalResources++
Copy link
Member Author

Choose a reason for hiding this comment

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

Incrementing when we send the result to the channel seems more clear to me than counting all the objects before the loop

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe worth adding a separate log message for relations if we don't have one

@github-actions github-actions bot added fix and removed fix labels Sep 20, 2022
@hermanschaaf
Copy link
Member

Counter-point: I think we should count relations, or at least report them; we're doing non-trivial work and API calls to fetch them. The ideal would be to report on relational table counts separately--not sure how easy that would be to do in practice

@erezrokah
Copy link
Member Author

Counter-point: I think we should count relations, or at least report them; we're doing non-trivial work and API calls to fetch them. The ideal would be to report on relational table counts separately--not sure how easy that would be to do in practice

πŸ’― Agree we should count them separately. Let me see if I can add it to this PR

@erezrokah erezrokah force-pushed the fix/dont_count_relations branch from b078932 to 97e7c73 Compare September 20, 2022 11:43
@erezrokah erezrokah changed the title fix: Don't count relations under total resources for top level tables fix: Print correct number of table resources Sep 20, 2022
@github-actions github-actions bot added fix and removed fix labels Sep 20, 2022
@erezrokah
Copy link
Member Author

erezrokah commented Sep 20, 2022

@hermanschaaf I changed the PR to print total for both relations and top level tables, and keep the total count a sum of all tables

@kodiakhq kodiakhq bot merged commit bcbd2a2 into cloudquery:main Sep 20, 2022
kodiakhq bot pushed a commit that referenced this pull request Sep 20, 2022
πŸ€– I have created a release *beep* *boop*
---


## [0.7.8](v0.7.7...v0.7.8) (2022-09-20)


### Bug Fixes

* Print correct number of table resources ([#143](#143)) ([bcbd2a2](bcbd2a2))

---
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 join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Question: Should we be counting resources from relations under the total count?

3 participants