Fix/GitHub issues sync improvements#1843
Conversation
…PRs from count and add is_pull_request?, is_issue? methods and scopes for clarity
|
@schneems it would be really great if you get chance to check this as we are planning to kickstart open source Friday event at our organization and codetriage is base thing for us to let people find the issues |
|
@prathamesh-sonpatki do review this if you get a chance to review |
|
Thanks for the PR. I didn't merge this as it had a lot of intertwined and (seemingly) unrelated changes and it was unclear why the fix was "the fix" i.e. the problem wasn't spelled out as to what would make you think that's the problem in such a way that I could validate that you've fixed it correctly (or alternatively help provide more information to you). #1841 is caused by hitting limits of the database as many tables are using "integer" and "serial" instead of "bigint" and "bigserial" (64bit integers). And by a failure in Database failureThere's not an easy way you could have guessed or known that without access to the database, so I don't fault you for not fixing, but it did take time to review the fix and determine that I didn't have enough information and needed to explore the problem myself. Last Url failureThe # If no next_url, we're on the last page
return true if response.next_url.nil?
# If next_url exists but last_url is nil, there are more pages
# (git_hub_bub gem bug - it tries to parse nil last_url)
return false if response.last_url.nil?But it wasn't clear that was the fix, or why. It's best served by fixing it "upstream" in the gem (which I also own) but this fix is okay too for a short term change. In the future, it would have been really helpful if the PR had just that change coupled with some hard output or evidence (reproducible) on how to validate what the bug was and that it was correctly fixed. I appreciate the pings. I missed the other issue come in and didn't catch that ALL issues new had stopped populating. So your PR was still helpful in raising awareness of a more serious issue. |
Description
This PR fixes several issues related to GitHub issues synchronization and Ruby 3.2 compatibility:
Ruby 3.2 Compatibility:
require "logger"inconfig/boot.rbandconfig/application.rb(Ruby 3.2 no longer auto-loads Logger)~> 5.0(version 6.0+ removedminitest/mock)Asset Pipeline Fix:
local-timeJavaScript asset path forlocal_timegem 3.x (local-time.es2017-umd)GitHub Issues Sync Improvements:
last_page?pagination handling whenlast_urlis nil (was causingURI::InvalidURIError)per_page=100parameter to fetch more issues per API request (reduced API calls)issues_countto exclude Pull Requests (GitHub's Issues API returns both issues and PRs)rate_limit_sleep!to avoid hitting GitHub API limitsper_pageconfigurable viaGITHUB_ISSUES_PER_PAGEenv variableCode Clarity:
is_pull_request?,is_issue?methods andpull_requests,issues_onlyscopes to Issue modelTest Fixes:
per_pageparameterlast_page?behaviorRelated Issue
Fixes #1841
Motivation and Context
issues_countwas including Pull Requests, showing 169 instead of 136 actual issuesPopulateIssuesJobwas failing withURI::InvalidURIErrorwhen GitHub didn't return alast_urlin pagination headersper_page=30required more API calls; now usesper_page=100(GitHub's maximum)How Has This Been Tested?
bin/rake test- 211 runs, 403 assertions, 0 failures, 0 errorsPopulateIssuesJobagainst real GitHub repos (aasm/aasm)GITHUB_ISSUES_PER_PAGEvalues to ensure configurabilityScreenshots (if appropriate):
Types of changes
Checklist: