revert me: Ignore github archive incremental sync for old integrations #2723
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IntegrationRunRepository
participant DataSource
Client->>IntegrationRunRepository: getIntegrationData(integrationId)
IntegrationRunRepository->>DataSource: Fetch integration data
alt results is null
IntegrationRunRepository-->>Client: return null
else IntegrationType is GITHUB and updatedAt < 2024-12-10
IntegrationRunRepository-->>Client: return null
else
IntegrationRunRepository-->>Client: return integration data
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
services/libs/data-access-layer/src/old/apps/integration_run_worker/integrationRun.repo.ts (1)
125-130: Temporary change needs trackingThe PR title suggests this is a temporary change ("revert me"). This should be:
- Documented with a TODO comment explaining when/why to revert
- Tracked in an issue for follow-up
if ( results.type === IntegrationType.GITHUB && moment(results.updatedAt).isBefore(moment('2024-12-10')) ) { + // TODO: Temporary change to ignore GitHub archive incremental sync for old integrations + // Track: <issue_url> return null }Would you like me to create a GitHub issue to track the reversion of this temporary change?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/libs/data-access-layer/src/old/apps/integration_run_worker/integrationRun.repo.ts(2 hunks)
🔇 Additional comments (2)
services/libs/data-access-layer/src/old/apps/integration_run_worker/integrationRun.repo.ts (2)
5-5: LGTM: Import addition is clean and necessary
The addition of IntegrationType to the existing import statement is well-organized and required for the new type checking logic.
121-123: LGTM: Proper null check added
The null check for results is a good defensive programming practice.
| if ( | ||
| results.type === IntegrationType.GITHUB && | ||
| moment(results.updatedAt).isBefore(moment('2024-12-10')) | ||
| ) { | ||
| return null | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Critical: Confirmed issues with GitHub integration date filtering
- The SQL query is missing the
updatedAtfield but it's used in the code, which will cause runtime errors - The hardcoded future date (Dec 10, 2024) appears only once in the codebase with no configuration alternative
- The class has logging capabilities (extends RepositoryBase with parentLog) but isn't using it for this important filtering
Locations needing attention:
services/libs/data-access-layer/src/old/apps/integration_run_worker/integrationRun.repo.ts:- Add
updatedAtto the SQL SELECT clause - Move the date to environment configuration
- Add debug logging when skipping integrations
- Add
The original review comment's suggestions are accurate and should be implemented to fix these issues.
🔗 Analysis chain
Critical: Several concerns with the GitHub integration date filtering
- The SQL query doesn't select the
updatedAtfield but it's used in the condition - The hardcoded future date (Dec 10, 2024) should be:
- Moved to a configuration value
- Documented with the reason for this specific date
- Ideally not set in the future
- Consider adding debug logging when skipping integrations
Suggested improvements:
select id,
platform as type,
status as state,
"integrationIdentifier" as identifier,
- "tenantId"
+ "tenantId",
+ "updatedAt"
from integrations where id = $(integrationId) and "deletedAt" is null
// ...
if (
results.type === IntegrationType.GITHUB &&
- moment(results.updatedAt).isBefore(moment('2024-12-10'))
+ moment(results.updatedAt).isBefore(moment(process.env.GITHUB_ARCHIVE_CUTOFF_DATE))
) {
+ this.log.debug('Skipping GitHub integration due to cutoff date', {
+ integrationId,
+ updatedAt: results.updatedAt
+ });
return null
}Let's verify if there are any other instances of this date being used:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other instances of this date in the codebase
rg -i "2024-12-10"
# Search for any environment variable definitions that might be related
rg "GITHUB_ARCHIVE_CUTOFF_DATE"
Length of output: 204
Script:
#!/bin/bash
# Let's verify if the updatedAt field is actually missing from the SQL query
ast-grep --pattern 'select $$$, "updatedAt" $$$ from integrations'
# Also check the full SQL query pattern to be thorough
ast-grep --pattern 'select $$$ from integrations where id = $(integrationId) and "deletedAt" is null'
# Check if there's any logging setup in this file
rg -A 2 "constructor.*log" services/libs/data-access-layer/src/old/apps/integration_run_worker/integrationRun.repo.ts
Length of output: 292
Script:
#!/bin/bash
# Let's try a different approach to find the SQL query
rg -A 10 "select.*from integrations.*where" services/libs/data-access-layer/src/old/apps/integration_run_worker/integrationRun.repo.ts
# Check for logger initialization in the class
rg "class.*IntegrationRunRepo" -A 10 services/libs/data-access-layer/src/old/apps/integration_run_worker/integrationRun.repo.ts
Length of output: 994
Changes proposed ✍️
What
copilot:summary
copilot:poem
Why
How
copilot:walkthrough
Checklist ✅
Feature,Improvement, orBug.Summary by CodeRabbit
Bug Fixes
Refactor
getIntegrationDatamethod for better data integrity.