Add retryable transport for errors #1704

dcfranca · 2023-05-29T12:39:59Z

In order to address the issue #210
I have added 3 new parameters to the provider

retry_delay_ms
max_retries
retryable_errors

In case max_retries is > 0 (defaults to zero)
it will retry the request in case it is an error
the retryable_errors defaults to [500, 502, 503, 504]

It retries after the ms specified in retry_delay_ms (defaults to 1000)

Resolves #210

Behavior

Before the change?

When there are intermittent errors on the GitHub API the provider fails directly

After the change?

You should be able to set different behaviors to handle failures from the GitHub API, retrying specific errors and delaying for next retry

Other information

Additional info

Pull request checklist

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
[] Added the appropriate label for the given change

Does this introduce a breaking change?

The code should work as before in case no max_retries is set

Please see our docs on breaking changes to help!

Yes (Please add the Type: Breaking change label)
No

If Yes, what's the impact:

N/A

Pull request type

Please add the corresponding label for change this PR introduces:
I don't think I have permission to add a label here

Bugfix: Type: Bug
Feature/model/API additions: Type: Feature
Updates to docs or samples: Type: Documentation
Dependencies/code cleanup: Type: Maintenance

In order to address the issue integrations#210 I have added 3 new parameters to the provider - retry_delay_ms - max_retries - retryable_errors In case max_retries is > 0 (defaults to zero) it will retry the request in case it is an error the retryable_errors defaults to [500, 502, 503, 504] It retries after the ms specified in retry_delay_ms (defaults to 1000)

github/provider.go

kfcampbell

Thank you for your attention to testing! This will be a cool feature to get released.

github/config.go

github/transport_test.go

kfcampbell · 2023-06-01T21:03:04Z

@dcfranca what are your thoughts on handling the error to silence the lint warning?

…github into add-retries

dcfranca · 2023-06-03T05:14:09Z

@dcfranca what are your thoughts on handling the error to silence the lint warning?

Done

dcfranca · 2023-06-06T14:32:54Z

@kfcampbell
Do I need to do something else for the awaiting approvals?

dcfranca · 2023-06-08T09:06:35Z

@kfcampbell @ilmax Hey, any update on this?

dcfranca · 2023-06-12T07:33:56Z

@kfcampbell @ilmax Hi, is there anything else I need to do here?

ilmax · 2023-06-12T07:43:41Z

Hey @dcfranca I have no power here 😅 I'm just an end user, @kfcampbell is the one you're looking for.
On a side note, I saw that it takes a while before PR gets reviewed and merged so just sit back, 🍿 relax and wait for this to be reviewed/merged 😄

github/transport.go

…github into add-retries

kfcampbell · 2023-06-26T20:57:19Z

github/transport.go

+
+	for retry := 0; retry <= t.maxRetries; retry++ {
+		// Reset the body
+		// Code from httpretry (https://github.com/ybbus/httpretry/blob/master/roundtripper.go#L60)


Can you talk to me a little bit about this code and why you chose it?

How was this tested?

The tests I have added on transport_test.go were failing due to the body not being reset after the first retry, then I checked the package I use on some of my projects to retry (httpretry) and saw that they have a code for this scenario, so I shameless copied it and the tests succeed

I didn't figure out how to test the specific scenario of using the function GetBody, the original library also only tests without the GetBody, but if you have an idea on how to test with it I can update the tests

Do you have a manual workload you can test this on that will reliably trigger a 500? I'm hesitant to merge/release to a wide audience without a bit more description of the validation process.

I have a Flux integration that quite often gives me a 500 (a few times a day), I can try linking my branch instead of the default provider there... however, I have never integrated an in development provider, so I will appreciate anything that can guide me through this
I'm also a bit afraid of doing that on this workload, it is not critical, but I would expect there are more changes here that are not tested/stable

You can create the binary for this provider and point Terraform towards it by seeing our debugging instructions.

We do not have successful automated integration testing at the moment so we've been over-relying on manual testing. I don't feel super comfortable merging without manually testing this extensively first.

github/transport.go

Co-authored-by: Keegan Campbell <me@kfcampbell.com>

gm-hyp · 2023-09-28T14:09:16Z

Hello! When do you believe this will get merged? This behaviour is seen when refreshing/creating the resource github_emu_group_mapping linked to a team larger than 1500 people. It is kinda painful as it fails about 50% of the time with this error:


Error: GET https://api.github.com/orgs/<hidden>/external-group/<hidden>: 502 Server Error []

  with github_emu_group_mapping.<hidden>,
  on <hidden>.tf line 7, in resource "github_emu_group_mapping" "<hidden>":
   7: resource "github_emu_group_mapping" "<hidden>" {

Error: Terraform exited with code 1.
Error: Process completed with exit code 1.

The only way to make it succeed is to retry 3-4-5 times. Having a way to specify a retry mechanism would help immensely. Thank you for the work done here!

dcfranca · 2023-10-03T12:52:14Z

Hello! When do you believe this will get merged? This behaviour is seen when refreshing/creating the resource github_emu_group_mapping linked to a team larger than 1500 people. It is kinda painful as it fails about 50% of the time with this error:
Error: GET https://api.github.com/orgs/<hidden>/external-group/<hidden>: 502 Server Error []

  with github_emu_group_mapping.<hidden>,
  on <hidden>.tf line 7, in resource "github_emu_group_mapping" "<hidden>":
   7: resource "github_emu_group_mapping" "<hidden>" {

Error: Terraform exited with code 1.
Error: Process completed with exit code 1.
The only way to make it succeed is to retry 3-4-5 times. Having a way to specify a retry mechanism would help immensely. Thank you for the work done here!

I'd love to know as well :D

kfcampbell · 2023-10-17T17:35:22Z

@gm-hyp @dcfranca we'd love to release a major version in the near future (next couple of months?) pending our team's capacity to do so. We have several items we're looking at merging into that version that we'll need to test together thoroughly. Thanks for your persistence here.

kfcampbell · 2024-01-10T23:35:07Z

I've changed the target branch of this PR to the v6 branch. I'll create a PR shortly from that branch to main, and use that branch as the location for manual testing of the features to be added in the next major version.

kfcampbell · 2024-01-10T23:35:54Z

See #2091.

* Add retryable transport for errors (#1704) * Add retryable transport for errors In order to address the issue #210 I have added 3 new parameters to the provider - retry_delay_ms - max_retries - retryable_errors In case max_retries is > 0 (defaults to zero) it will retry the request in case it is an error the retryable_errors defaults to [500, 502, 503, 504] It retries after the ms specified in retry_delay_ms (defaults to 1000) * Update documentation with new parameters for retry * Change default of max_retries to 3 * Fix typo in naming * Update github/transport_test.go * Add error check for data seek * Consolidate the default retriable errors on a function * Fix typo on the comments Co-authored-by: Keegan Campbell <me@kfcampbell.com> * Update vendor * Fix merging conflicts * Update documentation with new parameters for retry * Change default of max_retries to 3 * Fix typo in naming * Add error check for data seek * Update github/transport_test.go * Consolidate the default retriable errors on a function * Fix typo on the comments Co-authored-by: Keegan Campbell <me@kfcampbell.com> * Don't run go mod tidy on release (#1788) * Don't run go mod tidy on release * Be more specific about releases * Fix lint with APIMeta deprecation --------- Co-authored-by: Keegan Campbell <me@kfcampbell.com> Co-authored-by: Nick Floyd <139819+nickfloyd@users.noreply.github.com> * fix: remove repository topic from state if it doesnt exist in GitHub anymore (#1918) * remove repository topic if they cannot be found in GitHub anymore * Fix build error by bumping package version in offending file --------- Co-authored-by: Keegan Campbell <me@kfcampbell.com> * Bump version to v6 (#2106) * Upgrade to Terraform Plugin SDK v2 (#1780) * go mod tidy -go=1.16 && go mod tidy -go=1.17 * Run go mod vendor * Attempt v2 upgrade * Plugin compiling * Fix some provider test errors * Fix test compilation error * ValidateFunc --> ValidateDiagFunc * Fix casing * Sprinkle toDiagFunc everywhere * More fixes for validation functions * State --> StateContext * %s --> %v when printing diags * ConfigureFunc --> ConfigureContextFunc * Checking results of d.Set, round one * Continue checking d.Set results * Check results of d.Set, round three * Checking d.Set results, round four * d.Set round five * In tests, export GITHUB_TEST_ORGANIZATION * Remove unnecessary MaxItems on computed value * Go build now works * Resolve linting errors * Apply diag.FromErr twice more * Pass key names into toDiagFunc helper * Construct cty.Path from strings * Tests now working * Update terraform-plugin-sdk version * Remove commented attribute setting in resource_github_team.go * Fix restrict pushes on github_branch_protection. Fix branch protection tests (#2045) * Update restrict pushes. Fix branch protection tests Change blocks_creations default to true. Fix broken build. * add state migration * rename push_restrictions to push_allowances * correct state migration issue * add docs clarification * update migration func args * fix test args * cleanup tests * Pass context.Background() in test * fix timestamp fields --------- Co-authored-by: Keegan Campbell <me@kfcampbell.com> * Set group_id correctly (#2133) * Run go get -u github.com/golangci/golangci-lint * Separate github_team_members import from github_team as create_default_maintainers is not defined for members resource (#2126) Co-authored-by: Keegan Campbell <me@kfcampbell.com> * Add missing variable definition for test case --------- Co-authored-by: Daniel França <github.t6297kgphp.dv@koderama.com> Co-authored-by: Nick Floyd <139819+nickfloyd@users.noreply.github.com> Co-authored-by: Felix Luthman <34520175+felixlut@users.noreply.github.com> Co-authored-by: georgekaz <1391828+georgekaz@users.noreply.github.com> Co-authored-by: Rich Young <richjyoung@users.noreply.github.com>

dcfranca added 2 commits May 29, 2023 14:39

Update documentation with new parameters for retry

5ae5432

ilmax reviewed May 30, 2023

View reviewed changes

github/provider.go Show resolved Hide resolved

dcfranca added 2 commits May 31, 2023 19:28

Change default of max_retries to 3

df9f8d5

Fix typo in naming

e41f935

dcfranca requested a review from ilmax May 31, 2023 18:16

kfcampbell approved these changes Jun 1, 2023

View reviewed changes

github/config.go Show resolved Hide resolved

github/transport_test.go Outdated Show resolved Hide resolved

kfcampbell added 2 commits June 1, 2023 13:55

Update github/transport_test.go

0da71b9

Merge branch 'main' into add-retries

0b3ba89

dcfranca added 2 commits June 3, 2023 07:12

Add error check for data seek

c2c341c

Merge branch 'add-retries' of github.com:dcfranca/terraform-provider-…

e31cac0

…github into add-retries

Merge branch 'main' into add-retries

88e9ae8

dcfranca added 4 commits June 15, 2023 14:39

Merge branch 'main' into add-retries

2aa7bfd

Merge branch 'main' into add-retries

bb97a82

Merge branch 'main' into add-retries

61d9a17

Merge branch 'main' into add-retries

27ded6c

kfcampbell reviewed Jun 21, 2023

View reviewed changes

github/transport.go Outdated Show resolved Hide resolved

dcfranca and others added 4 commits June 22, 2023 19:11

Consolidate the default retriable errors on a function

218287f

Merge branch 'add-retries' of github.com:dcfranca/terraform-provider-…

bd787ef

…github into add-retries

Merge branch 'main' into add-retries

9b98c08

Merge branch 'main' into add-retries

6d9b78d

kfcampbell reviewed Jun 26, 2023

View reviewed changes

github/transport.go Outdated Show resolved Hide resolved

Fix typo on the comments

c2f8580

Co-authored-by: Keegan Campbell <me@kfcampbell.com>

dcfranca added 12 commits August 10, 2023 09:41

Merge branch 'main' into add-retries

6ba0735

Merge branch 'main' into add-retries

f4274eb

Merge branch 'main' into add-retries

e9534c7

Merge branch 'main' into add-retries

b40c2b7

Merge branch 'main' into add-retries

61e196a

Merge branch 'main' into add-retries

bf61de6

Merge branch 'main' into add-retries

7de4a84

Merge branch 'main' into add-retries

679e033

Merge branch 'main' into add-retries

564ed52

Merge branch 'main' into add-retries

8465c92

Merge branch 'main' into add-retries

3221167

Merge branch 'main' into add-retries

c1b5e49

dcfranca and others added 5 commits October 3, 2023 14:52

Merge branch 'main' into add-retries

ef547ad

Merge branch 'main' into add-retries

4d63684

Merge branch 'main' into add-retries

053fd0b

Merge branch 'main' into add-retries

fff38a1

Merge branch 'main' into add-retries

4f27468

Merge branch 'main' into add-retries

23b3302

nickfloyd added the Type: Breaking change Used to note any change that requires a major version bump label Dec 11, 2023

Merge branch 'main' into add-retries

a2c18d9

kfcampbell changed the base branch from main to v6 January 10, 2024 23:28

Fix lint with APIMeta deprecation

23634d2

kfcampbell merged commit 2dddeb3 into integrations:v6 Jan 10, 2024
1 check passed

kfcampbell mentioned this pull request Jan 10, 2024

v6 #2091

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add retryable transport for errors #1704

Add retryable transport for errors #1704

dcfranca commented May 29, 2023 •

edited

kfcampbell left a comment

kfcampbell commented Jun 1, 2023

dcfranca commented Jun 3, 2023

dcfranca commented Jun 6, 2023

dcfranca commented Jun 8, 2023

dcfranca commented Jun 12, 2023

ilmax commented Jun 12, 2023

kfcampbell Jun 26, 2023

kfcampbell Jun 26, 2023

dcfranca Jun 27, 2023 •

edited

kfcampbell Jun 27, 2023

dcfranca Jun 28, 2023

kfcampbell Jun 28, 2023

gm-hyp commented Sep 28, 2023

dcfranca commented Oct 3, 2023

kfcampbell commented Oct 17, 2023

kfcampbell commented Jan 10, 2024

kfcampbell commented Jan 10, 2024

Add retryable transport for errors #1704

Add retryable transport for errors #1704

Conversation

dcfranca commented May 29, 2023 • edited

Behavior

Before the change?

After the change?

Other information

Additional info

Pull request checklist

Does this introduce a breaking change?

Pull request type

kfcampbell left a comment

Choose a reason for hiding this comment

kfcampbell commented Jun 1, 2023

dcfranca commented Jun 3, 2023

dcfranca commented Jun 6, 2023

dcfranca commented Jun 8, 2023

dcfranca commented Jun 12, 2023

ilmax commented Jun 12, 2023

kfcampbell Jun 26, 2023

Choose a reason for hiding this comment

kfcampbell Jun 26, 2023

Choose a reason for hiding this comment

dcfranca Jun 27, 2023 • edited

Choose a reason for hiding this comment

kfcampbell Jun 27, 2023

Choose a reason for hiding this comment

dcfranca Jun 28, 2023

Choose a reason for hiding this comment

kfcampbell Jun 28, 2023

Choose a reason for hiding this comment

gm-hyp commented Sep 28, 2023

dcfranca commented Oct 3, 2023

kfcampbell commented Oct 17, 2023

kfcampbell commented Jan 10, 2024

kfcampbell commented Jan 10, 2024

dcfranca commented May 29, 2023 •

edited

dcfranca Jun 27, 2023 •

edited