Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove OAuth authentication for GitHub #159

Merged
merged 4 commits into from
Dec 17, 2021
Merged

Conversation

nedbat
Copy link
Contributor

@nedbat nedbat commented Dec 15, 2021

Also, improve the error handling to be sure we can see what's happening when an exception occurs.

@nedbat nedbat requested a review from a team December 15, 2021 22:10
@nedbat
Copy link
Contributor Author

nedbat commented Dec 15, 2021

@Carlos-Muniz @kdmccormick These changes make the bot work well for the openedx organization.

Ned Batchelder added 3 commits December 17, 2021 11:36
The move from edx to openedx broke the OAuth authentication because the
edx org had no OAuth restrictions, but the openedx org does.  We don't
need OAuth, so just use the personal access token.
Copy link
Contributor

@Carlos-Muniz Carlos-Muniz left a comment

Choose a reason for hiding this comment

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

There is some coverage missing. Can tests be written for these?

Comment on lines +33 to +39
try:
ret = pull_request_changed(pull_request)
log_rate_limit()
except Exception as exc:
logger.exception("Couldn't pull_request_changed_task")
raise

Copy link
Contributor

Choose a reason for hiding this comment

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

Missing coverage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh snap! :)

Comment on lines +81 to +84
except Exception as exc:
req = response.request
logger.exception(f"HTTP request failed: {req.method} {req.url}. Response body: {response.content}")
raise
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing coverage

Comment on lines +89 to +91
rate = get_github_session().get("/rate_limit").json()['rate']
reset = time.strftime("%Y-%m-%d %H:%M:%S", time.localtime(rate['reset']))
logger.info(f"Rate limit: {rate['limit']}, used {rate['used']}, remaining {rate['remaining']}. Reset is at {reset}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing coverage

Comment on lines 172 to 173
if callable(callback):
callback(resp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing Coverage

@nedbat nedbat merged commit 96c1e8f into master Dec 17, 2021
@nedbat nedbat deleted the nedbat/debug-openedx-fail branch December 17, 2021 17:56
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.

3 participants