-
Notifications
You must be signed in to change notification settings - Fork 26
feat: Support downloading plugins from other cloudquery repos #632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
⏱️ Benchmark resultsComparing with 5a97453
|
registry/download.go
Outdated
| resp.Body.Close() | ||
| return i, nil | ||
| } | ||
| time.Sleep(RetryWaitTime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not keep retry logic as-is and just don't retry on 404? Also with the logic above, looks like for each retry URLs would start from index 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until now we have retried on 404s, so I wanted to keep that behavior the same. But maybe we shouldn't 🤷 Though 404s can happen for other reasons, like the plugin version still being in the process of being published. So with this updated logic things are exactly the same as before, but if it's the cloudquery repo we also check the secondary url on 404 before sleeping for the next try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The retry logic was added due to #291 (503 GitHub errors).
I don't think we've seen other error codes so I think we don't need to retry on 404
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the context @erezrokah, I'll update this to not retry on 404s, but fall back if there are more urls to try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated 👍 I also added an integration test that tries out the various cases
Codecov ReportBase: 46.73% // Head: 47.15% // Increases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #632 +/- ##
==========================================
+ Coverage 46.73% 47.15% +0.42%
==========================================
Files 69 70 +1
Lines 6591 6721 +130
==========================================
+ Hits 3080 3169 +89
- Misses 3078 3106 +28
- Partials 433 446 +13
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 at Codecov. |
🤖 I have created a release *beep* *boop* --- ## [1.33.0](v1.32.0...v1.33.0) (2023-02-01) ### Features * Support downloading plugins from other cloudquery repos ([#632](#632)) ([9e1501e](9e1501e)) ### Bug Fixes * **deps:** Update golang.org/x/exp digest to f062dba ([#641](#641)) ([c6ec154](c6ec154)) * **deps:** Update google.golang.org/genproto digest to 1c01626 ([#642](#642)) ([fc9f338](fc9f338)) * **deps:** Update module github.com/avast/retry-go/v4 to v4.3.2 ([#643](#643)) ([2f6a2e8](2f6a2e8)) * **deps:** Update module github.com/getsentry/sentry-go to v0.17.0 ([#644](#644)) ([fb33f8c](fb33f8c)) * **deps:** Update module github.com/rs/zerolog to v1.29.0 ([#645](#645)) ([e864963](e864963)) * **deps:** Update module github.com/schollz/progressbar/v3 to v3.13.0 ([#646](#646)) ([c2146d3](c2146d3)) * **deps:** Update module golang.org/x/net to v0.5.0 ([#647](#647)) ([417c99d](417c99d)) * **deps:** Update module golang.org/x/text to v0.6.0 ([#649](#649)) ([a91c7dc](a91c7dc)) * **deps:** Update module google.golang.org/grpc to v1.52.3 ([#650](#650)) ([48d96ee](48d96ee)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
|
Opened #655 as a follow up as I'm not sure the current URL logic for community plugins is what we want |
Right now, if a plugin is hosted in anywhere on the
cloudqueryorg, it can only be downloaded if it is part of thecloudquery/cloudquerymonorepo. This makes it impossible to temporarily hostcq-source-*type plugins as othercloudqueryrepos, as can be done with community plugins on other orgs.This changes the download logic to first check the monorepo, but fall back to the community standard of
cq-source-*if it's A)cloudqueryorg and B) the monorepo download attempt resulted in a 404. If both fail with 404, the attempt is aborted. Other status codes are retried.It would have been better if we could know, somehow, when a plugin is hosted in the monorepo or outside it, but right now this is not possible unless we change the spec in some way.
This change allows us to use the CLI to download https://github.com/cloudquery/cq-source-simple-analytics