Skip to content

Added fix for GitHub's transient fault issue - #234

Open
akkumar-qlik wants to merge 3 commits into
masterfrom
SAC-31798-discovery-401-errors-fix
Open

Added fix for GitHub's transient fault issue#234
akkumar-qlik wants to merge 3 commits into
masterfrom
SAC-31798-discovery-401-errors-fix

Conversation

@akkumar-qlik

Copy link
Copy Markdown

Description of change

https://qlik-dev.atlassian.net/browse/SAC-31798
Added BadCredentialsException to the backoff retry tuple in client.py:203.
Added inline rationale in client.py:199 documenting why 401 retries are needed for transient GitHub faults.
Updated unit test expectation for 401 retry behavior in

Manual QA steps

  • Discovery: Running
  • Sync: Running
  • Unit Tests: Running
  • Integration Tests: Running

Risks

Rollback steps

  • revert this branch

AI generated code

https://internal.qlik.dev/general/ways-of-working/code-reviews/#guidelines-for-ai-generated-code

  • this PR has been written with the help of GitHub Copilot or another generative AI tool

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the GitHub API client retry behavior to better tolerate GitHub’s documented transient 401 responses, helping discovery/sync complete successfully during short-lived API-side incidents.

Changes:

  • Added BadCredentialsException (401) to the authed_get backoff retry exceptions and documented the rationale inline.
  • Updated unit test expectations to assert 401s are retried up to max_tries=5.
  • Bumped package version to 3.4.1 and documented the change in CHANGELOG.md.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
tap_github/client.py Adds 401 (BadCredentialsException) to backoff retry tuple and documents why.
tests/unittests/test_exception_handling.py Updates expected retry call count for 401 responses and adjusts test docstring.
setup.py Bumps package version from 3.4.0 to 3.4.1.
CHANGELOG.md Adds release notes for 3.4.1 describing the transient 401 retry change.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines 77 to 81
"""
- Verify that `authed_get` raises an error with the proper message for different error codes.
- Verify that tap retries 5 times for Server5xxError and RateLimitExceeded error.
- Verify that tap retries 5 times for Server5xxError, TooManyRequests,
and transient 401 BadCredentialsException.
"""

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added the fix

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.

2 participants