feat: add github_snowflake plugin for Snowflake-backed GitHub ingestion - #139
feat: add github_snowflake plugin for Snowflake-backed GitHub ingestion#139mfrancisc wants to merge 15 commits into
Conversation
Ingest GitHub PR data from Fivetran GITHUB_DB.MARTS into existing _tool_github_* tables and reuse adapted GitHub convertors, following the jira_snowflake pattern for per-repo historical sync without API rate limits. Co-Authored-By: Cursor <noreply@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 1:36 PM UTC · Completed 1:55 PM UTC |
PR Summary by QodoAdd github_snowflake plugin to ingest GitHub PR data from Snowflake (Fivetran MARTS)
AI Description
Diagram
High-Level Assessment
Files changed (33)
|
Code Review by Qodo
1.
|
![qodo-app-for-konflux-ci[bot]](https://avatars.githubusercontent.com/u/155583288?s=60&v=4){kind=small}
| /* | ||
| Licensed to the Apache Software Foundation (ASF) under one or more | ||
| contributor license agreements. See the NOTICE file distributed with | ||
| this work for additional information regarding copyright ownership. | ||
| The ASF licenses this file to You under the Apache License, Version 2.0 |
There was a problem hiding this comment.
3. Files changed outside owned dirs 📘 Rule violation ⌂ Architecture
This PR changes/adds files outside the only allowed owned-plugin directories (backend/plugins/aireview/, backend/plugins/codecov/, backend/plugins/testregistry/). This violates the upstream-modification restriction and may create unsupported divergences.
Agent Prompt
## Issue description
The PR modifies/adds files outside the only permitted owned plugin directories.
## Issue Context
Compliance rule 1105 allows modifications only within `backend/plugins/aireview/`, `backend/plugins/codecov/`, or `backend/plugins/testregistry/`.
## Fix Focus Areas
- backend/plugins/github_snowflake/impl/impl.go[1-5]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Previous run (5)ReviewFindingsMedium
Low
Labels: Large new plugin PR (44 files, 3400+ lines) requiring thorough human review across auth handling, Snowflake integration, and domain model conversion. Next steps:
Previous run (6)ReviewFindingsMedium
Low
Next steps:
Previous run (7)ReviewFindingsMedium
Low
Next steps:
Previous run (8)ReviewFindingsMedium
Low
Next steps:
Previous run (9)Looks good to me — well-structured new plugin that follows the established ReviewFindingsLow
Previous run (10)ReviewFindingsMedium
Low
Labels: PR adds a new Snowflake-backed GitHub data ingestion plugin Previous run (11)ReviewFindingsHigh
Medium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
rsoaresd
left a comment
rsoaresd
left a comment
There was a problem hiding this comment.
Excellent work 🚀 Still testing locally, but it seems all good 🚀 Just minor suggestions:
| timeAfter := cmd.Flags().StringP("timeAfter", "a", "", "only sync records created/updated after this time (RFC3339)") | ||
|
|
||
| cmd.Run = func(cmd *cobra.Command, args []string) { | ||
| runner.DirectRun(cmd, args, PluginEntry, map[string]interface{}{ |
There was a problem hiding this comment.
should we also pass the connectionId here?
| ### Minimal real-run setup | ||
|
|
||
| 1. Start MySQL: `podman compose -f docker-compose-dev.yml up -d mysql` | ||
| 2. Ensure `.env` has `DB_URL`, `ENCRYPTION_SECRET`, and (for local API) `AUTH_ENABLED=false` |
There was a problem hiding this comment.
Not sure if it is relevant, but env.example mentions DB_URL=mysql://merico:merico@mysql:3306/lake?charset=utf8mb4&parseTime=True&loc=UTC, I needed to change to DB_URL=mysql://merico:merico@localhost:3306/lake?charset=utf8mb4&parseTime=True&loc=UTC
There was a problem hiding this comment.
makes sense, I've improved a bit the docs , see 7b025bf
Encrypt Snowflake private keys at rest via gorm encdec (including jira_snowflake), handle nullable PR updated_at, and tighten AuthType and owner/repo validation. Co-Authored-By: Cursor Grok 4.5 <noreply@example.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · |
Require connectionId for standalone runs (and jira_snowflake) and note that native make run needs localhost in DB_URL. Co-Authored-By: Cursor Grok 4.5 <noreply@example.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · ❌ Terminated · Started 10:40 AM UTC · Ended 10:57 AM UTC |
|
Code review by qodo was updated up to the latest commit 7b025bf |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 10:40 AM UTC · Completed 10:56 AM UTC |
Document that github migrations must create shared _tool_github_* tables before pipelines run, and add troubleshooting for the common failures. Co-Authored-By: Cursor Grok 4.5 <noreply@example.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · ❌ Terminated · Started 12:18 PM UTC · Ended 12:37 PM UTC |
|
Code review by qodo was updated up to the latest commit 171f31b |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
|
🤖 Finished Review · ✅ Success · Started 12:18 PM UTC · Completed 12:37 PM UTC |
Add console verification steps and troubleshooting for missing role grants before local plugin testing. Co-Authored-By: Cursor Grok 4.5 <noreply@example.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · ❌ Terminated · Started 12:51 PM UTC · Ended 1:06 PM UTC |
|
Code review by qodo was updated up to the latest commit 64fdf81 |
|
Code review by qodo was updated up to the latest commit cf67860 |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 9:53 AM UTC · Completed 10:07 AM UTC |
Align SyncAccounts with other sync tasks so tool-layer account rows carry _raw_data_params for full-sync domain cleanup. Co-Authored-By: Cursor <noreply@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · ❌ Terminated · Started 11:29 AM UTC · Ended 11:46 AM UTC |
|
Code review by qodo was updated up to the latest commit 0f1041e |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 11:29 AM UTC · Completed 11:46 AM UTC |
|
🤖 Finished Review · ✅ Success · Started 1:50 PM UTC · Completed 2:08 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
requires-manual-review
| repoId := data.Options.GithubId | ||
| fullName := data.Options.Name | ||
|
|
||
| query, args := buildAccountsQuery(repoId) |
There was a problem hiding this comment.
buildAccountsQuery takes no timeAfter parameter, so every run fetches all accounts. Is it the plan for the downstream convertor to reconvert all accounts every run? Not a blocker at pilot scale but might be worth adding a timestamp filter and conditional update before scaling.
There was a problem hiding this comment.
That is intentional for now since a naive timeAfter on user rows won’t shrink much, most of the cost is the repo_users UNION over PR/review history.
Also I think accounts don’t have a natural “updated for this repo” timestamp and filtering on USER/USER_EMAIL alone can miss new associations (same user appears on a new PR).
I would leave it unfiltered for the time being, and improve/optimize it later if we observe any issues/slowness.
WDYT?
There was a problem hiding this comment.
Makes sense — good point about the missing associations. Agree its fine to leave as-is for now.
| u.LOGIN AS author_name, | ||
| im.MERGED_AT, | ||
| im.ACTOR_ID AS merged_by_id, | ||
| mu.LOGIN AS merged_by_name |
There was a problem hiding this comment.
Since they are missing from MARTS additions, deletions, and comment counts aren't in this SELECT. Some of our dashboard metrics use PR size (additions/deletions) and we may like to use them more going forward. Is there any future path to getting these from Fivetran?
There was a problem hiding this comment.
That's is a gap in the available data indeed. For this we will have to ask the Fivetran data platform whether additions/deletions (and comment totals) can be added to MARTS or a side table.
But I haven't started this conversation yet with that team.
rsoaresd
left a comment
rsoaresd
left a comment
There was a problem hiding this comment.
Amazing work!! 🚀 I tested with GITHUB_MARTS_GROUP role and it works very well!
…tream-diffs conflict Make ConvertAccounts filter on gra.repo_github_id with an INNER JOIN so the query is unambiguous. Keep both the snowflakehelper and pkg/ notes when resolving the merge conflict in docs/upstream-diffs.md. Co-Authored-By: Cursor Grok 4.6 <noreply@example.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 10:30 AM UTC · Completed 10:45 AM UTC Commit: |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 2:01 PM UTC · Completed 2:21 PM UTC Commit: |
| // ParseRSAPrivateKey parses a PKCS#8 PEM-encoded RSA private key. | ||
| func ParseRSAPrivateKey(pemStr string) (*rsa.PrivateKey, error) { | ||
| block, _ := pem.Decode([]byte(pemStr)) | ||
| if block == nil { |
There was a problem hiding this comment.
[medium] error-handling-gap
Open returns *sql.DB without calling db.Ping() to verify the connection is reachable. sql.Open only validates DSN syntax; bad credentials or unreachable Snowflake host surface as confusing errors on the first sync query. Matches existing jira_snowflake pattern and is standard Go database/sql behavior.
Suggested fix: Consider adding db.PingContext(ctx) after sql.Open to fail fast on configuration errors. This requires accepting a context.Context parameter.
| - **Scope unit is `GithubRepo`** (numeric `githubId` + `fullName` owner/repo). | ||
| - **No raw-table layer**: writes directly to `_tool_github_*`. Sync tasks populate | ||
| `RawDataOrigin` (`_raw_data_table` / `_raw_data_params`) on tool-layer rows so | ||
| convertors can delete domain records by `_raw_data_params` on full sync |
There was a problem hiding this comment.
[low] operational-constraint
Documented constraint that a repo must not be configured in both a GitHub API connection and a github_snowflake connection is not enforced at runtime. Domain ID duplication could result.
Suggested fix: Consider adding a runtime check in PrepareTaskData that queries existing GithubRepo scopes for the same githubId on a different connection type.
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
There was a problem hiding this comment.
[low] missing-DependencyTables
Sync subtask metas do not declare DependencyTables or ProductTables. The framework cannot track table ownership for sync tasks. Matches the jira_snowflake pattern.
Suggested fix: Add ProductTables declarations to sync subtask metas.
| if since != nil { | ||
| clauses = append(clauses, dal.Where("_tool_github_accounts.updated_at >= ?", since)) | ||
| } | ||
| } |
There was a problem hiding this comment.
[low] test-coverage-gap
convertOrphanedRepoAccounts uses a LEFT JOIN with IS NULL filter but has no unit tests for the orphan-handling logic.
Suggested fix: Add unit tests for the orphan repo accounts handling.
| return &op, nil | ||
| } | ||
|
|
||
| // validateOwnerRepo checks that name is in "owner/repo" format. |
There was a problem hiding this comment.
[low] validation-message-format
Validation error messages inconsistently include the invalid value. connectionId includes the value via fmt.Sprintf, but name does not.
Suggested fix: Update all validation errors to consistently include or exclude the invalid value.
4 participants
Jira: https://redhat.atlassian.net/browse/DPROD-1374
Summary
github_snowflakeplugin that reads GitHub data from Fivetran Snowflake (GITHUB_DB.MARTS) and writes into existing_tool_github_*tables, then runs adapted GitHub convertors (same pattern asjira_snowflake).externalbrowserauth, migrations, unit tests for query builders, local testing guide, and owned-plugin registration indocs/upstream-diffs.md.Scope / trade-offs