Skip to content

feat: Move proto to external repository#844

Merged
kodiakhq[bot] merged 7 commits intomainfrom
feat/update_to_plugin_pb
May 8, 2023
Merged

feat: Move proto to external repository#844
kodiakhq[bot] merged 7 commits intomainfrom
feat/update_to_plugin_pb

Conversation

@yevgenypats
Copy link
Contributor

@yevgenypats yevgenypats commented May 3, 2023

This removes protobuf files and generated code to https://github.com/cloudquery/plugin-pb/ and https://github.com/cloudquery/plugin-pb-go/.

This will make it possible for the CLI not import the plugin-sdk (any other clients) as well as decouple version upgrade from protobuf updates and SDK updates.

This is ready for first review but Im still working on a PR for the CLI to test that it all works properly before merging.

@yevgenypats yevgenypats requested a review from hermanschaaf May 3, 2023 15:47
@github-actions github-actions bot added the feat label May 3, 2023
@github-actions
Copy link

github-actions bot commented May 3, 2023

⏱️ Benchmark results

  • DefaultConcurrencyDFS-2 resources/s: 10,329
  • DefaultConcurrencyRoundRobin-2 resources/s: 12,348
  • Glob-2 ns/op: 299.6
  • TablesWithChildrenDFS-2 resources/s: 24,414
  • TablesWithChildrenRoundRobin-2 resources/s: 22,520
  • TablesWithRateLimitingDFS-2 resources/s: 28.24
  • TablesWithRateLimitingRoundRobin-2 resources/s: 863.9

@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -1.10 ⚠️

Comparison is base (2df4413) 47.81% compared to head (bc75d1e) 46.72%.

❗ Current head bc75d1e differs from pull request most recent head 2963399. Consider uploading reports for the commit 2963399 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #844      +/-   ##
==========================================
- Coverage   47.81%   46.72%   -1.10%     
==========================================
  Files          76       59      -17     
  Lines        7865     6583    -1282     
==========================================
- Hits         3761     3076     -685     
+ Misses       3599     3106     -493     
+ Partials      505      401     -104     
Impacted Files Coverage Δ
internal/backends/local/local.go 59.45% <ø> (ø)
internal/memdb/memdb.go 83.66% <ø> (ø)
plugins/source/plugin.go 41.14% <ø> (ø)
plugins/source/scheduler_dfs.go 64.04% <ø> (ø)
plugins/source/scheduler_round_robin.go 84.93% <ø> (ø)
plugins/source/testing.go 12.90% <ø> (ø)
serve/destination.go 54.61% <100.00%> (ø)
serve/source.go 58.94% <100.00%> (ø)

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

if err := json.Unmarshal(resourceB, &resource); err != nil {
t.Fatalf("failed to unmarshal resource: %v", err)
if err := json.Unmarshal(r.Resource, &resource); err != nil {
t.Fatal(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
t.Fatal(err)
t.Fatalf("failed to unmarshal resource: %v", err)

if err != nil {
t.Fatalf("Failed to dial bufnet: %v", err)
}
c, err := clients.NewClient(ctx, specs.RegistryGrpc, "", "", clients.WithGRPCConnection(conn), clients.WithNoSentry())
Copy link
Contributor

Choose a reason for hiding this comment

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

How does the --no-sentry option get passed to the plugins now? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the code for managing the plugins will now live in the CLI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the test because it's already running sentry is not relevant here.

if err := json.Unmarshal(resourceB, &resource); err != nil {
t.Fatalf("failed to unmarshal resource: %v", err)
if err := json.Unmarshal(r.Resource, &resource); err != nil {
t.Fatal(err)
Copy link
Contributor

@hermanschaaf hermanschaaf May 3, 2023

Choose a reason for hiding this comment

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

Suggested change
t.Fatal(err)
t.Fatalf("failed to unmarshal resource: %v", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it's just test then I prefer to to add formatting because it printing the line anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

some places had formatting and some didn't so I tried to make it consistent without formatting in test and just call t.Fatal

@github-actions github-actions bot added feat and removed feat labels May 3, 2023
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Copy link
Contributor

@hermanschaaf hermanschaaf left a comment

Choose a reason for hiding this comment

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

Haven't tested it myself yet, but looks good from a read-through. Nothing too scary :)

@kodiakhq kodiakhq bot merged commit 3cd3ba7 into main May 8, 2023
@kodiakhq kodiakhq bot deleted the feat/update_to_plugin_pb branch May 8, 2023 14:04
kodiakhq bot pushed a commit that referenced this pull request May 8, 2023
🤖 I have created a release *beep* *boop*
---


## [2.6.0](v2.5.4...v2.6.0) (2023-05-08)


### Features

* **arrow:** Add `types.XBuilder.NewXArray` helpers ([2df4413](2df4413))
* Move proto to external repository ([#844](#844)) ([3cd3ba7](3cd3ba7))

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