Closed harishv7 closed 7 months ago
lgtm, minor nits!
Could I also clarify what the rollout plan of this is? Are we planning to do a swapover for only comments at a later date (once we ensure that no remaining open PRs on github are open with comments), or are we planning to phase out the open PRs on github as well? Currently we still make use of the pull request system on Github even though we also maintain our own internal record of PRs
users
,reviewers
andsequelize
seem to be unused here!Nit:
ReviewMeta
andReviewRequest
are unused
@alexanderleegs The changes are compatible with GitHub comments. So it pulls from both GitHub and DB before returning to FE.
However, new comments are only created in our DB. So in the future, say 1-2 months when we are relatively confident most of the open RRs (before the date of merge) are closed, we can remove the logic to fetch from GH - saves 1 network call as well
@alexanderleegs The changes are compatible with GitHub comments. So it pulls from both GitHub and DB before returning to FE.
However, new comments are only created in our DB. So in the future, say 1-2 months when we are relatively confident most of the open RRs (before the date of merge) are closed, we can remove the logic to fetch from GH - saves 1 network call as well
This rollout plan is sane, just confirming here, this would not require any manual remediation right? since there will be no deuplciates since a review has its comments either stored in gh OR db, never gh AND db, right?
just double checking here - the intent of this PR is to complete phase out github?
NOTE: This PR requires DB migration to add new table
Problem
Task: We rely on GitHub to manage comments. Right now, comments are not linked to a specific file nor line on a changed file. The comments apply to the entire RR. This is easy to replicate within CMS BE.
Solution
What we need is:
A new table to store comments (as strings). The rough fields are RR_ID, User_ID, Site_ID, Comment (String)
Impact: Eliminates dependency on GitHub API Requests, Pull Requests, and consequently GitHub Token usage
Note: For now, the logic will fetch comments from both GitHub + DB till we completely phase out GitHub usage, as there may be some already in-progress RRs which have comments stored on GitHub
Breaking Changes
Tests
Deploy Notes
Note: A DB migration is required for this PR.