Skip to content

feat: Add retry logic when downloading plugins from GitHub#310

Merged
erezrokah merged 9 commits intocloudquery:mainfrom
cychiang:feat/add-retry-when-downloading-plugins
Nov 3, 2022
Merged

feat: Add retry logic when downloading plugins from GitHub#310
erezrokah merged 9 commits intocloudquery:mainfrom
cychiang:feat/add-retry-when-downloading-plugins

Conversation

@cychiang
Copy link
Copy Markdown
Contributor

@cychiang cychiang commented Oct 25, 2022 •
edited by erezrokah
Loading

Summary

Fixes #291

  • Add retry with retry-go library
  • Start retry when status code != 200
  • By default, it will retry 5 times and wait for 1 second between each attempt.

Here is the example output if status code != 200:

failed downloading: https://github.com/cloudquery/cloudquery/releases/download/plugins-source-my-test-v1.1.5/test_darwin_amd64.zip: All attempts fail:
#1: statusCode != 200
#2: statusCode != 200
#3: statusCode != 200
#4: statusCode != 200
#5: statusCode != 200

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 ✅

@github-actions github-actions bot added the feat label Oct 25, 2022
erezrokah
erezrokah requested changes Oct 27, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @cychiang! WDYT about using https://github.com/avast/retry-go for the retry logic?

I think it could simplify the code and also avoid the need to write test cases as we rely on an already tested library.

Please let me know that you think

@erezrokah erezrokah mentioned this pull request Oct 27, 2022
Closed
@cychiang
Copy link
Copy Markdown
Contributor Author

cychiang commented Oct 27, 2022

Thanks for the PR @cychiang! WDYT about using https://github.com/avast/retry-go for the retry logic?

I think it could simplify the code and also avoid the need to write test cases as we rely on an already tested library.

Please let me know that you think

Yes, I do also thinking about the library you mentioned, focusing on the retry itself. But go-resty supports save response into file in the other hand might be helpful to simplify the codebase? But if the project would like to support downloading plugins other than GitHub in the future then I think would be good to consider which one would be good in the next step. Maybe I just make this issue more complicate and confuse 😶‍🌫️

@erezrokah
Copy link
Copy Markdown
Member

erezrokah commented Oct 27, 2022

Yes, I do also thinking about the library you mentioned, focusing on the retry itself. But go-resty supports save response into file in the other hand might be helpful to simplify the codebase? But if the project would like to support downloading plugins other than GitHub in the future then I think would be good to consider which one would be good in the next step. Maybe I just make this issue more complicate and confuse 😶‍🌫️

That's a very good point about go-resty. How about we try with retry-go as that would be a smaller change and will resolve #291, and open a new issue to consider using go-resty so we can discuss the benefits of that approach?

I can open the issue to use go-resty or you can do that too and explain the benefits. Please let me know what you prefer

@cychiang
Copy link
Copy Markdown
Contributor Author

cychiang commented Oct 27, 2022

Yes, I do also thinking about the library you mentioned, focusing on the retry itself. But go-resty supports save response into file in the other hand might be helpful to simplify the codebase? But if the project would like to support downloading plugins other than GitHub in the future then I think would be good to consider which one would be good in the next step. Maybe I just make this issue more complicate and confuse 😶‍🌫️

That's a very good point about go-resty. How about we try with retry-go as that would be a smaller change and will resolve #291, and open a new issue to consider using go-resty so we can discuss the benefits of that approach?

I can open the issue to use go-resty or you can do that too and explain the benefits. Please let me know what you prefer

Sounds good, let's put go-resty to another issue and focus on retry-go for the #291 😄

@github-actions github-actions bot added feat and removed feat labels Oct 28, 2022
@github-actions github-actions bot added feat and removed feat labels Oct 28, 2022
@cychiang cychiang marked this pull request as ready for review October 28, 2022 18:51
@cychiang cychiang requested a review from yevgenypats as a code owner October 28, 2022 18:51
@github-actions github-actions bot added feat and removed feat labels Oct 28, 2022
@cychiang cychiang requested a review from erezrokah October 28, 2022 18:56
@github-actions github-actions bot added feat and removed feat labels Oct 28, 2022
erezrokah
erezrokah approved these changes Oct 30, 2022
View reviewed changes
Copy link
Copy Markdown
Member

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

This looks great, thanks for the follow up @cychiang 👍

I'll let @yevgenypats comment too

@erezrokah
Copy link
Copy Markdown
Member

erezrokah commented Nov 3, 2022 •
edited
Loading

I'll go ahead and merge (and release this) as it fixes the bug. We can do any follow in future PRs.

One thing I noticed is that the error is printed at the end of all the retry attempts. It would be better to print the attempt on each iteration, which I fixed in a4a4f5f, see:

Loading spec(s) from examples/config-azure.yml
Failed downloading https://github.com/cloudquery/cloudquery/releases/download/plugins-source-azure-v1.2.0/azure_darwin_arm64.zip with status code <code>. Retrying
Failed downloading https://github.com/cloudquery/cloudquery/releases/download/plugins-source-azure-v1.2.0/azure_darwin_arm64.zip with status code <code>. Retrying
Failed downloading https://github.com/cloudquery/cloudquery/releases/download/plugins-source-azure-v1.2.0/azure_darwin_arm64.zip with status code <code>. Retrying
Failed downloading https://github.com/cloudquery/cloudquery/releases/download/plugins-source-azure-v1.2.0/azure_darwin_arm64.zip with status code <code>. Retrying
Failed downloading https://github.com/cloudquery/cloudquery/releases/download/plugins-source-azure-v1.2.0/azure_darwin_arm64.zip with status code <code>. Retrying
Error: failed to get source plugin client for azure: failed to download plugin: failed downloading: https://github.com/cloudquery/cloudquery/releases/download/plugins-source-azure-v1.2.0/azure_darwin_arm64.zip

@erezrokah erezrokah merged commit 914d252 into cloudquery:main Nov 3, 2022
@github-actions github-actions bot added feat and removed feat labels Nov 3, 2022
@erezrokah
Copy link
Copy Markdown
Member

erezrokah commented Nov 3, 2022

Thanks for this PR @cychiang 🎸

@cq-bot cq-bot mentioned this pull request Nov 3, 2022
Merged
kodiakhq bot pushed a commit that referenced this pull request Nov 4, 2022
@cq-bot
chore(main): Release v0.13.20 (#343)
e4e6567 
🤖 I have created a release *beep* *boop*
---


## [0.13.20](v0.13.19...v0.13.20) (2022-11-04)


### Features

* Add retry logic when downloading plugins from GitHub ([#310](#310)) ([914d252](914d252))
* Enable Multiline table description ([#345](#345)) ([d83c60a](d83c60a))

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

Reviewers

@erezrokah erezrokah erezrokah approved these changes

@yevgenypats yevgenypats Awaiting requested review from yevgenypats

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feat: Add retry logic to plugins download

2 participants

@cychiang @erezrokah