Skip to content

Conversation

@cychiang
Copy link
Contributor

@cychiang cychiang commented Oct 25, 2022

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

@cychiang
Copy link
Contributor Author

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
Member

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

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

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

I'll let @yevgenypats comment too

@erezrokah
Copy link
Member

erezrokah commented Nov 3, 2022

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
Member

Thanks for this PR @cychiang 🎸

kodiakhq bot pushed a commit that referenced this pull request Nov 4, 2022
🤖 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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: Add retry logic to plugins download

2 participants