Skip to content

Conversation

@erezrokah
Copy link
Member

@erezrokah erezrokah commented Apr 18, 2023

Summary

Follow up to #790


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 βœ…

@erezrokah erezrokah requested a review from yevgenypats as a code owner April 18, 2023 16:35
@github-actions github-actions bot added the fix label Apr 18, 2023
@erezrokah erezrokah force-pushed the fix/fail_on_empty_tables branch from 6999422 to e93ae27 Compare April 18, 2023 16:35
@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 πŸŽ‰

Comparison is base (a61f34e) 46.63% compared to head (433729c) 46.65%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #796      +/-   ##
==========================================
+ Coverage   46.63%   46.65%   +0.02%     
==========================================
  Files          76       76              
  Lines        7820     7823       +3     
==========================================
+ Hits         3647     3650       +3     
  Misses       3682     3682              
  Partials      491      491              
Impacted Files Coverage Ξ”
specs/source.go 57.14% <100.00%> (+1.73%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

β˜” View full report in Codecov by Sentry.
πŸ“’ Do you have feedback about the report comment? Let us know in this issue.

@github-actions github-actions bot added fix and removed fix labels Apr 18, 2023
@github-actions
Copy link

github-actions bot commented Apr 18, 2023

⏱️ Benchmark results

  • DefaultConcurrencyDFS-2 resources/s: 10,365
  • DefaultConcurrencyRoundRobin-2 resources/s: 12,410
  • Glob-2 ns/op: 171.4
  • TablesWithChildrenDFS-2 resources/s: 27,790
  • TablesWithChildrenRoundRobin-2 resources/s: 27,413
  • TablesWithRateLimitingDFS-2 resources/s: 28.4
  • TablesWithRateLimitingRoundRobin-2 resources/s: 845.4
  • BufferedScanner-2 ns/op: 8.275
  • LogReader-2 ns/op: 27.07

Copy link
Contributor

@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.

small but important nit imo :)

specs/source.go Outdated
}

if len(s.Tables) == 0 {
return fmt.Errorf("tables configuration is required. Hint: try setting tables to [\"*\"] to sync all tables or use `cloudquery tables` to list available tables")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shouldn't recommend * because then we are back to square 1 in terms of usability. Hint: choose the tables you want to sync

Copy link
Member Author

Choose a reason for hiding this comment

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

πŸ’― Wasn't sure about that hint too. See 6ef586c

@erezrokah erezrokah requested a review from yevgenypats April 19, 2023 17:21
@kodiakhq kodiakhq bot merged commit 1320d32 into cloudquery:main Apr 20, 2023
erezrokah pushed a commit that referenced this pull request Apr 20, 2023
πŸ€– I have created a release *beep* *boop*
---


##
[2.3.8](v2.3.7...v2.3.8)
(2023-04-20)


### Bug Fixes

* Fail on empty tables
([#796](#796))
([1320d32](1320d32))
* **testing:** Add sorting for testing dest migrations
([#814](#814))
([b1437f1](b1437f1))

---
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.

2 participants