Fix github API handling and keyman behavior to correct handling of invalid or missing keys #417
Merged
Conversation
…AND that its value is not None Signed-off-by: Adrian Edwards <adredwar@redhat.com>
…vailable Signed-off-by: Adrian Edwards <adredwar@redhat.com>
…ing an exception that will invalidate the key This should help interrupt the facade starvation pipeline. Signed-off-by: Adrian Edwards <adredwar@redhat.com>
Signed-off-by: Adrian Edwards <adredwar@redhat.com>
"Ensure that apps retry the request at least once after receiving a 401 error, but with a slight delay to allow the database to catch up." https://github.com/orgs/community/discussions/101661#discussioncomment-8342211 Signed-off-by: Adrian Edwards <adredwar@redhat.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| self.__handle_github_not_authorized_response() | ||
| raise last_exception | ||
|
|
||
| def _decide_retry_policy(exception: Exception) -> bool: |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
E0213: Method '_decide_retry_policy' should have "self" as first argument (no-self-argument)
| @@ -156,7 +156,7 @@ def new_key(self, platform: str) -> str | None: | |||
| if not len(self.fresh_keys[platform]): | |||
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
C1802: Do not use len(SEQUENCE) without comparison to determine if a sequence is empty (use-implicit-booleaness-not-len)
| @@ -156,7 +156,7 @@ def new_key(self, platform: str) -> str | None: | |||
| if not len(self.fresh_keys[platform]): | |||
| if not len(self.expired_keys[platform]): | |||
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
C1802: Do not use len(SEQUENCE) without comparison to determine if a sequence is empty (use-implicit-booleaness-not-len)
MoralCode
added
the
high priority
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR is sort of a catch-all for a couple problems that I suspect were a large part of the cause behind #391
Fix 1: some none handling bugs in keyman
A couple small bugs related to handling of
Nonevalues in the key handling (keyman) part of the code (improper condition and a barereturnstatement). Fixed the condition and changed the return to a WaitKeyException to request that the caller sleep for 5 minutes and try again later to see if theres any keys that have refreshed.Bug 2: Improper handling of 401 HTTP errors in Github API code
The github REST API helper class (
GithubDataAccess) was permanently invalidating a key upon receiving a 401 unauthorized response. This was despite the key in question still remaining valid when i manually checked it sometime later. My best guess is its an erroneous/transient error code from github. The way the code handled these errors is contrary to some advice i found online from a github employee suggesting that 401's should be retried a few times after a delay before treating the key as permanently invalidated. So the fix implemented for this is to move that invalidation step outside the retry loop, meaning we only invalidate a key if the unauthorized exception happens on the last retry (suggesting that the 9 prior retries also failed)Notes for Reviewers
This has been tested repeatedly alongside #415 on a test instance and i dont believe there are issues with it
Signed commits
Generative AI disclosure
Please select one option:
If AI tools were used, please provide details below:
- What tools were used? Sonnet 4.6 Medium via cursor
- How were these tools used? to the extent they were used at all (i dont remember), it would have been mainly for diagnosing the issue and maybe some incidental in-IDE code completion/helping.
- Did you review these outputs before submitting this PR? yes. i maintained situational awareness and control over every code decision at all times.