load projects on push #1964
load projects on push #1964
Conversation
WalkthroughThe changes refactor how projects are associated with repositories, shifting from using repository IDs and struct references to using repository full name strings throughout the backend. GitHub push events now trigger asynchronous project loading when pushes occur on the default branch. GitLab push event handling is removed. Several database methods and logging statements are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub
participant Controller as GithubAppWebHook
participant Handler as handlePushEvent
participant Services as LoadProjectsFromGithubRepo
participant DB
GitHub->>Controller: PushEvent webhook
Controller->>Controller: Log event
Controller->>Handler: Async handlePushEvent(payload)
Handler->>Handler: Extract repo info & check default branch
Handler->>Services: LoadProjectsFromGithubRepo(...)
Services->>DB: Lookup installation, repo, org
Services->>GitHub: Clone repo at branch
Services->>Services: Load digger config
Services->>DB: Refresh projects from config
Services-->>Handler: Success or error
Handler-->>Controller: Log errors if any
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 5
🔭 Outside diff range comments (1)
backend/models/orgs.go (1)
86-120: 🛠️ Refactor suggestionRemove unused fields from the JSON response structure.
The JSON struct still includes fields that are no longer populated:
RepoID,RepoName,RepoOrg, andRepoUrl. These will be sent as zero/empty values in the response, which could confuse API consumers.Consider removing the unused fields from the JSON struct:
return struct { Id uint `json:"id"` Name string `json:"name"` Directory string `json:"directory"` OrganisationID uint `json:"organisation_id"` OrganisationName string `json:"organisation_name"` - RepoID uint `json:"repo_id"` RepoFullName string `json:"repo_full_name"` - RepoName string `json:"repo_name"` - RepoOrg string `json:"repo_org"` - RepoUrl string `json:"repo_url"` IsInMainBranch bool `json:"is_in_main_branch"` IsGenerated bool `json:"is_generated"` LastActivityTimestamp string `json:"last_activity_timestamp"` LastActivityAuthor string `json:"last_activity_author"` LastActivityStatus string `json:"last_activity_status"` }
🧹 Nitpick comments (3)
backend/controllers/runs.go (1)
60-66: Additional database call introduced for repository fetching.The change to explicitly fetch the repository using
GetRepoByFullNameis consistent with the refactoring to use repository full names instead of embedded repository references. The error handling is properly implemented.Consider the performance impact of this additional database call. If this endpoint is frequently accessed, you might want to:
- Cache repository lookups at the application level
- Optimize the database query with proper indexing on the full name field
- Consider batch fetching if multiple projects are processed
#!/bin/bash # Description: Check for similar patterns that might benefit from optimization # Find other instances where GetRepoByFullName might be called frequently rg "GetRepoByFullName" -A 3 -B 3 # Check for potential caching mechanisms rg -i "cache.*repo|repo.*cache" --type gobackend/services/repos.go (1)
23-25: Include original error in the returned error message for better debugging.-slog.Error("Error getting GetGithubAppInstallationLink: %v", "installationId", installationId, "error", err) -return fmt.Errorf("error getting github app link") +slog.Error("Error getting GetGithubAppInstallationLink", "installationId", installationId, "error", err) +return fmt.Errorf("error getting github app link: %w", err)backend/models/storage.go (1)
244-244: Fix inconsistent logging field name.There's an inconsistency in logging field names. Line 244 uses
"repoFullname"while other places (like line 222) use"repoFullName". Please standardize the field name for consistency.Apply this diff to maintain consistency:
- "repoFullname", repoFullName, + "repoFullName", repoFullName,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/controllers/github.go(2 hunks)backend/controllers/policies.go(1 hunks)backend/controllers/projects.go(2 hunks)backend/controllers/runs.go(3 hunks)backend/models/orgs.go(2 hunks)backend/models/storage.go(7 hunks)backend/services/repos.go(1 hunks)ee/backend/controllers/gitlab.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/services/repos.go (1)
libs/digger_config/digger_config.go (1)
LoadDiggerConfigYaml(481-533)
backend/models/storage.go (4)
backend/models/setup.go (2)
Database(13-15)DB(20-20)backend/models/orgs.go (3)
Project(74-84)Repo(16-27)Organisation(9-14)libs/digger_config/config.go (1)
Project(44-61)libs/digger_config/yaml.go (1)
DiggerConfigYaml(9-26)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (13)
backend/controllers/policies.go (1)
250-254:✅ Verification successful
LGTM: Consistent with repository refactoring approach.
The change to use
RepoFullNameinstead ofRepoIDaligns with the broader refactoring to decouple projects from repository ID references.Please verify that
repoModel.RepoFullNameis properly populated before this assignment. Run this script to check the RepoFullName field usage:
🏁 Script executed:
#!/bin/bash # Description: Verify RepoFullName field is properly handled in Repo model # Check how RepoFullName is populated in the Repo struct ast-grep --pattern 'type Repo struct { $$$ RepoFullName $_ $$$ }' # Check for RepoFullName assignments rg "RepoFullName.*=" -A 2 -B 2Length of output: 7238
RepoFullName field verification complete
We’ve confirmed that
repoModel.RepoFullNameis correctly defined and populated:
- The
Repostruct includes aRepoFullName stringfield in:
ee/drift/model/repos.gen.gonext/model/repos.gen.gobackend/models/orgs.go- Generated ORM code in
next/models_generated/repos.gen.goandee/drift/models_generated/repos.gen.goassignsRepoFullNamefrom therepo_full_namecolumn.- GORM’s default loading behavior hydrates all struct fields, so fetching a
Reporecord will populateRepoFullNamebefore it’s used inupsertPolicyForRepoAndProject.No further action required.
backend/controllers/projects.go (2)
254-254: LGTM: Updated logging to use new repository field structure.The change from
project.Repo.Nametoproject.RepoFullNamemaintains the same logging information while aligning with the new Project struct that uses repository full name strings instead of repository object references.
359-359: LGTM: Project creation updated for new repository association.The use of
RepoFullNamefield in Project creation is consistent with the broader refactoring to replace repository ID/struct references with repository full name strings.ee/backend/controllers/gitlab.go (1)
97-97:❌ Incorrect review comment
Verify intentional removal of GitLab push event handling.
The removal of GitLab push event processing represents a significant functional change that could affect users relying on this capability.
Please confirm this is an intentional architectural decision. Run this script to verify the scope of GitLab push event removal:
If this removal is intentional, consider:
- Documenting the migration path for users currently using GitLab push events
- Adding a clear deprecation notice in release notes
- Ensuring GitHub push event handling provides equivalent functionality
🏁 Script executed:
#!/bin/bash # Description: Check for remaining GitLab push event handling references # Search for any remaining GitLab push event handling rg -i "push.*event.*gitlab|gitlab.*push.*event" --type go # Check for handlePushEvent function references rg "handlePushEvent" --type go # Look for GitLab push event related imports or types rg "PushEvent.*gitlab|gitlab.*PushEvent" --type goLength of output: 1164
🏁 Script executed:
#!/bin/bash # Show context around GitLab PushEvent case to verify if any handling remains rg -n "gitlab.PushEvent" -C5 --type go ee/backend/controllers/gitlab.goLength of output: 441
GitLab PushEvent handling remains intact
The code in
ee/backend/controllers/gitlab.gostill includes acase *gitlab.PushEvent(lines 112–113), so GitLab push event processing was not removed. Please disregard the suggestion to verify its removal.Likely an incorrect or invalid review comment.
backend/controllers/runs.go (2)
76-78: LGTM: Updated to use explicitly fetched repository.The changes to use the explicitly fetched
repo.IDinstead ofproject.Repo.IDare consistent with the new repository association approach and maintain proper functionality.
97-97: LGTM: Logging updated for new repository reference.The logging change to use
repo.IDinstead of the previousproject.Repo.IDmaintains the same information while aligning with the refactored data access pattern.backend/controllers/github.go (2)
103-109: LGTM! Asynchronous push event handling is appropriate.The implementation correctly handles push events asynchronously to avoid webhook timeouts. The logging provides good visibility into the event processing.
393-418: Good error handling with panic recovery.The function properly handles panics and logs errors. Note that since this runs asynchronously after the webhook returns, any errors won't affect the webhook response status. Consider implementing a retry mechanism or alerting for critical failures in project loading.
backend/models/orgs.go (1)
79-79: Good simplification of the data model.Replacing the
Reporelationship with a simpleRepoFullNamestring field reduces coupling and simplifies the model structure.backend/models/storage.go (4)
218-250: LGTM! Function refactored correctly to use repository full name.The changes successfully refactor the function to use
repoFullNamestring instead of*Repostruct, which aligns with the broader codebase changes. The removal ofPreload("Repo")and the simplified WHERE clause usingrepo_full_namedirectly is consistent with the Project model now having aRepoFullNamefield.
380-410: LGTM! Well-implemented new function following existing patterns.The new
GetRepoByFullNamefunction is correctly implemented with proper error handling, logging, and follows the same patterns as the existingGetRepofunction. It provides the necessary functionality to retrieve repositories by their full name instead of just the name field.
1405-1432: LGTM! Function signature and implementation updated correctly.The
CreateProjectfunction has been successfully refactored to userepoFullNamestring instead of*Repostruct. The changes are consistent with the updated Project model that now has aRepoFullNamefield, and the logging statements have been updated accordingly.
868-868:❌ Incorrect review comment
Verify consistency with repository refactoring.
This line still uses
repo_id = ?in the WHERE clause, which may be inconsistent with the broader refactoring that's moving from repository IDs to repository full names. Please verify if theDiggerRunmodel should also be updated to userepoFullNameinstead ofrepo_id.
🏁 Script executed:
#!/bin/bash # Description: Check if DiggerRun model and related code still uses repo_id vs repoFullName # Check DiggerRun model definition ast-grep --pattern 'type DiggerRun struct { $$$ }' # Check other usages of repo_id in the codebase rg -A 3 -B 3 "repo_id.*=" --type go # Check if there are other references to repo_id that might need updating rg "repo_id" --type go | head -20Length of output: 22132
Leave repo_id filter unchanged
The
DiggerRunmodel still defines and uses therepo_idfield, so the existingWhere("project_name = ? AND repo_id = ?", projectName, repoId)clause is correct and does not need to be updated to
repoFullName.Likely an incorrect or invalid review comment.
| func LoadProjectsFromGithubRepo(gh utils2.GithubClientProvider, installationId string, repoFullName string, repoOwner string, repoName string, cloneUrl string, branch string) error { | ||
| installationId64, err := strconv.ParseInt(installationId, 10, 64) | ||
| if err != nil { | ||
| slog.Error("failed to convert installation id %v to int64", "insallationId", installationId) |
There was a problem hiding this comment.
Fix typo in log message.
-slog.Error("failed to convert installation id %v to int64", "insallationId", installationId)
+slog.Error("failed to convert installation id to int64", "installationId", installationId)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| slog.Error("failed to convert installation id %v to int64", "insallationId", installationId) | |
| slog.Error("failed to convert installation id to int64", "installationId", installationId) |
🤖 Prompt for AI Agents
In backend/services/repos.go at line 18, there is a typo in the log message key
"insallationId". Correct the typo by changing "insallationId" to
"installationId" to ensure the log key is spelled correctly.
| "github.com/diggerhq/digger/ee/drift/utils" | ||
| dg_configuration "github.com/diggerhq/digger/libs/digger_config" | ||
| utils2 "github.com/diggerhq/digger/next/utils" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use more descriptive import aliases for better readability.
The current aliases utils2 and utils3 are confusing. Consider using descriptive names that indicate their purpose.
-import (
- "fmt"
- "github.com/diggerhq/digger/backend/models"
- utils3 "github.com/diggerhq/digger/backend/utils"
- "github.com/diggerhq/digger/ee/drift/utils"
- dg_configuration "github.com/diggerhq/digger/libs/digger_config"
- utils2 "github.com/diggerhq/digger/next/utils"
- "log/slog"
- "strconv"
- "strings"
-)
+import (
+ "fmt"
+ "github.com/diggerhq/digger/backend/models"
+ backendutils "github.com/diggerhq/digger/backend/utils"
+ driftutils "github.com/diggerhq/digger/ee/drift/utils"
+ dg_configuration "github.com/diggerhq/digger/libs/digger_config"
+ ghprovider "github.com/diggerhq/digger/next/utils"
+ "log/slog"
+ "strconv"
+ "strings"
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "github.com/diggerhq/digger/ee/drift/utils" | |
| dg_configuration "github.com/diggerhq/digger/libs/digger_config" | |
| utils2 "github.com/diggerhq/digger/next/utils" | |
| import ( | |
| "fmt" | |
| "github.com/diggerhq/digger/backend/models" | |
| backendutils "github.com/diggerhq/digger/backend/utils" | |
| driftutils "github.com/diggerhq/digger/ee/drift/utils" | |
| dg_configuration "github.com/diggerhq/digger/libs/digger_config" | |
| ghprovider "github.com/diggerhq/digger/next/utils" | |
| "log/slog" | |
| "strconv" | |
| "strings" | |
| ) |
🤖 Prompt for AI Agents
In backend/services/repos.go around lines 7 to 9, the import aliases `utils2`
and `utils3` are unclear and reduce code readability. Rename these aliases to
more descriptive names that reflect the functionality or purpose of the imported
packages, such as using `driftutils` for the drift utils package or `nextutils`
for the next utils package, to improve clarity and maintainability.
backend/services/repos.go
Outdated
| err = utils3.CloneGitRepoAndDoAction(cloneUrl, branch, "", *token, "", func(dir string) error { | ||
| config, err := dg_configuration.LoadDiggerConfigYaml(dir, true, nil) | ||
| if err != nil { | ||
| slog.Error("ERROR load digger.yml: %v", "error", err) |
There was a problem hiding this comment.
Use appropriate log level and remove redundant ERROR prefix.
-slog.Error("ERROR load digger.yml: %v", "error", err)
+slog.Error("failed to load digger.yml", "error", err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| slog.Error("ERROR load digger.yml: %v", "error", err) | |
| slog.Error("failed to load digger.yml", "error", err) |
🤖 Prompt for AI Agents
In backend/services/repos.go at line 48, the log statement uses slog.Error with
a redundant "ERROR" prefix in the message and an incorrect format for passing
the error. Change the log level to a more appropriate one if needed (e.g., Warn
or Error based on context), remove the "ERROR" prefix from the message, and use
structured logging by passing the error as a key-value pair without formatting
strings.
| func (db *Database) RefreshProjectsFromRepo(orgId string, config configuration.DiggerConfigYaml, repoFullName string) error { | ||
| slog.Debug("UpdateRepoDiggerConfig, repo", "repoFullName", repoFullName) | ||
|
|
||
| org, err := db.GetOrganisationById(orgId) | ||
| if err != nil { | ||
| slog.Error("failed to get organisation", | ||
| "orgId", orgId, | ||
| "error", err) | ||
| return err | ||
| return fmt.Errorf("error retrieving org by name: %v", err) | ||
| } | ||
|
|
||
| err = db.GormDB.Transaction(func(tx *gorm.DB) error { | ||
| if isMainBranch { | ||
| // we reset all projects already in main branch to create new projects | ||
| repoProjects, err := db.GetProjectByRepo(orgId, repo) | ||
| if err != nil { | ||
| slog.Error("could not get repo projects", | ||
| "repoId", repo.ID, | ||
| "orgId", orgId, | ||
| "error", err) | ||
| return fmt.Errorf("could not get repo projects: %v", err) | ||
| } | ||
|
|
||
| slog.Info("resetting main branch flag for existing projects", | ||
| "projectCount", len(repoProjects), | ||
| "repoId", repo.ID) | ||
|
|
||
| for _, rp := range repoProjects { | ||
| rp.IsInMainBranch = false | ||
| err = db.UpdateProject(&rp) | ||
| if err != nil { | ||
| slog.Error("could not update existing main branch project", | ||
| "projectId", rp.ID, | ||
| "error", err) | ||
| return fmt.Errorf("could not update existing main branch projects: %v", err) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| slog.Info("processing projects from config", | ||
| "projectCount", len(config.Projects), | ||
| "repoId", repo.ID) | ||
|
|
||
| for _, dc := range config.Projects { | ||
| projectName := dc.Name | ||
| p, err := db.GetProjectByName(orgId, repo, projectName) | ||
| p, err := db.GetProjectByName(orgId, repoFullName, projectName) | ||
| if err != nil { | ||
| slog.Error("error retrieving project by name", | ||
| "projectName", projectName, | ||
| "repoId", repo.ID, | ||
| "error", err) | ||
| return fmt.Errorf("error retrieving project by name: %v", err) | ||
| } | ||
|
|
||
| if p == nil { | ||
| slog.Info("creating new project", | ||
| "projectName", projectName, | ||
| "repoId", repo.ID, | ||
| "isGenerated", dc.Generated, | ||
| "isMainBranch", isMainBranch) | ||
|
|
||
| _, err := db.CreateProject(projectName, org, repo, dc.Generated, isMainBranch) | ||
| _, err := db.CreateProject(projectName, org, repoFullName, false, true) | ||
| if err != nil { | ||
| slog.Error("could not create project", | ||
| "projectName", projectName, | ||
| "error", err) | ||
| return fmt.Errorf("could not create project: %v", err) | ||
| } | ||
| } else { | ||
| slog.Info("updating existing project", | ||
| "projectId", p.ID, | ||
| "projectName", projectName, | ||
| "isGenerated", dc.Generated, | ||
| "isMainBranch", isMainBranch) | ||
|
|
||
| if isMainBranch == true { | ||
| p.IsInMainBranch = isMainBranch | ||
| } | ||
| p.IsGenerated = dc.Generated | ||
| db.UpdateProject(p) | ||
| } | ||
| } | ||
| return nil | ||
| }) | ||
|
|
||
| if err != nil { | ||
| slog.Error("error while updating projects from config", | ||
| "repoId", repo.ID, | ||
| "error", err) | ||
| return fmt.Errorf("error while updating projects from config: %v", err) | ||
| } | ||
|
|
||
| slog.Info("successfully updated repo digger config", | ||
| "repoId", repo.ID, | ||
| "projectCount", len(config.Projects)) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review the simplified logic for potential issues.
The function has been successfully refactored and simplified, but there are a few concerns:
- Hardcoded
isInMainBranch: true: All created projects are set to be in the main branch, which may not always be accurate. - No updates to existing projects: The function only creates new projects but doesn't update existing ones. This might be the intended behavior, but please verify.
- Outdated comment: Line 1635 still references the old function name "UpdateRepoDiggerConfig".
Consider applying this diff to fix the comment:
- slog.Debug("UpdateRepoDiggerConfig, repo", "repoFullName", repoFullName)
+ slog.Debug("RefreshProjectsFromRepo, repo", "repoFullName", repoFullName)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (db *Database) RefreshProjectsFromRepo(orgId string, config configuration.DiggerConfigYaml, repoFullName string) error { | |
| slog.Debug("UpdateRepoDiggerConfig, repo", "repoFullName", repoFullName) | |
| org, err := db.GetOrganisationById(orgId) | |
| if err != nil { | |
| slog.Error("failed to get organisation", | |
| "orgId", orgId, | |
| "error", err) | |
| return err | |
| return fmt.Errorf("error retrieving org by name: %v", err) | |
| } | |
| err = db.GormDB.Transaction(func(tx *gorm.DB) error { | |
| if isMainBranch { | |
| // we reset all projects already in main branch to create new projects | |
| repoProjects, err := db.GetProjectByRepo(orgId, repo) | |
| if err != nil { | |
| slog.Error("could not get repo projects", | |
| "repoId", repo.ID, | |
| "orgId", orgId, | |
| "error", err) | |
| return fmt.Errorf("could not get repo projects: %v", err) | |
| } | |
| slog.Info("resetting main branch flag for existing projects", | |
| "projectCount", len(repoProjects), | |
| "repoId", repo.ID) | |
| for _, rp := range repoProjects { | |
| rp.IsInMainBranch = false | |
| err = db.UpdateProject(&rp) | |
| if err != nil { | |
| slog.Error("could not update existing main branch project", | |
| "projectId", rp.ID, | |
| "error", err) | |
| return fmt.Errorf("could not update existing main branch projects: %v", err) | |
| } | |
| } | |
| } | |
| slog.Info("processing projects from config", | |
| "projectCount", len(config.Projects), | |
| "repoId", repo.ID) | |
| for _, dc := range config.Projects { | |
| projectName := dc.Name | |
| p, err := db.GetProjectByName(orgId, repo, projectName) | |
| p, err := db.GetProjectByName(orgId, repoFullName, projectName) | |
| if err != nil { | |
| slog.Error("error retrieving project by name", | |
| "projectName", projectName, | |
| "repoId", repo.ID, | |
| "error", err) | |
| return fmt.Errorf("error retrieving project by name: %v", err) | |
| } | |
| if p == nil { | |
| slog.Info("creating new project", | |
| "projectName", projectName, | |
| "repoId", repo.ID, | |
| "isGenerated", dc.Generated, | |
| "isMainBranch", isMainBranch) | |
| _, err := db.CreateProject(projectName, org, repo, dc.Generated, isMainBranch) | |
| _, err := db.CreateProject(projectName, org, repoFullName, false, true) | |
| if err != nil { | |
| slog.Error("could not create project", | |
| "projectName", projectName, | |
| "error", err) | |
| return fmt.Errorf("could not create project: %v", err) | |
| } | |
| } else { | |
| slog.Info("updating existing project", | |
| "projectId", p.ID, | |
| "projectName", projectName, | |
| "isGenerated", dc.Generated, | |
| "isMainBranch", isMainBranch) | |
| if isMainBranch == true { | |
| p.IsInMainBranch = isMainBranch | |
| } | |
| p.IsGenerated = dc.Generated | |
| db.UpdateProject(p) | |
| } | |
| } | |
| return nil | |
| }) | |
| if err != nil { | |
| slog.Error("error while updating projects from config", | |
| "repoId", repo.ID, | |
| "error", err) | |
| return fmt.Errorf("error while updating projects from config: %v", err) | |
| } | |
| slog.Info("successfully updated repo digger config", | |
| "repoId", repo.ID, | |
| "projectCount", len(config.Projects)) | |
| return nil | |
| } | |
| func (db *Database) RefreshProjectsFromRepo(orgId string, config configuration.DiggerConfigYaml, repoFullName string) error { | |
| slog.Debug("RefreshProjectsFromRepo, repo", "repoFullName", repoFullName) | |
| org, err := db.GetOrganisationById(orgId) | |
| if err != nil { | |
| return fmt.Errorf("error retrieving org by name: %v", err) | |
| } | |
| err = db.GormDB.Transaction(func(tx *gorm.DB) error { | |
| for _, dc := range config.Projects { | |
| projectName := dc.Name | |
| p, err := db.GetProjectByName(orgId, repoFullName, projectName) | |
| if err != nil { | |
| return fmt.Errorf("error retrieving project by name: %v", err) | |
| } | |
| if p == nil { | |
| _, err := db.CreateProject(projectName, org, repoFullName, false, true) | |
| if err != nil { | |
| return fmt.Errorf("could not create project: %v", err) | |
| } | |
| } | |
| } | |
| return nil | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("error while updating projects from config: %v", err) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In backend/models/storage.go from lines 1634 to 1663, update the debug log on
line 1635 to reflect the current function name RefreshProjectsFromRepo instead
of the outdated UpdateRepoDiggerConfig. Review the use of the hardcoded
isInMainBranch parameter set to true when creating projects and adjust it if
this does not always apply. Also, verify if existing projects need updates since
the current logic only creates new projects without modifying existing ones; if
updates are required, add the necessary code to handle them.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/migrations/20250530015921.sql (2)
3-3: Consistent comment phrasing
For clarity, use “on table” instead of “to table” in SQL comments:-- Create index "idx_project_org" on table "projects"
4-5: Ensure safe index rebuild and uniqueness semantics
- Consider
CREATE INDEX CONCURRENTLYto avoid a full table lock on large datasets.- In Postgres, unique indexes treat NULLs as distinct, so multiple rows with NULL
repo_full_namewon’t conflict. If that’s unintended, either makerepo_full_nameNOT NULL or use a partial index filtering out NULLs.- Verify that all lookup queries (e.g., by name + org + repo_full_name) will use this new index effectively.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
backend/migrations/20250530015921.sql(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
| -- Modify "projects" table | ||
| ALTER TABLE "public"."projects" DROP COLUMN "repo_id", ADD COLUMN "repo_full_name" text NULL; |
There was a problem hiding this comment.
Missing data migration and nullability considerations
Dropping repo_id will erase existing repo associations and adding a nullable repo_full_name leaves legacy rows with null. You need to backfill repo_full_name (e.g., via a JOIN on the repos table) before dropping repo_id, then enforce NOT NULL if your code assumes every project has a repo name.
🤖 Prompt for AI Agents
In backend/migrations/20250530015921.sql at lines 1 to 2, the migration drops
the repo_id column and adds a nullable repo_full_name column without preserving
existing data. To fix this, first add the repo_full_name column as nullable,
then backfill it by joining the projects table with the repos table to populate
repo_full_name based on repo_id. After ensuring all rows have repo_full_name
set, alter the column to be NOT NULL if required, and finally drop the repo_id
column.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/services/repos.go (1)
18-18: Fix typo in log message key.There is a typo in the log message key "insallationId" - it should be "installationId".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/controllers/github_test.go(1 hunks)backend/models/orgs.go(2 hunks)backend/models/scheduler_test.go(1 hunks)backend/services/repos.go(1 hunks)backend/tasks/runs_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/models/orgs.go
🧰 Additional context used
🪛 golangci-lint (1.64.8)
backend/services/repos.go
51-51: Error return value of models.DB.RefreshProjectsFromRepo is not checked
(errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (5)
backend/services/repos.go (1)
15-59: Function logic looks correct overall.The function properly handles the workflow of:
- Converting installation ID to int64
- Getting GitHub installation link
- Retrieving repository information
- Getting GitHub service token
- Cloning repository and loading Digger configuration
- Refreshing projects in the database
The error handling is comprehensive and follows good practices with structured logging.
🧰 Tools
🪛 golangci-lint (1.64.8)
51-51: Error return value of
models.DB.RefreshProjectsFromRepois not checked(errcheck)
backend/controllers/github_test.go (1)
617-617: LGTM! Consistent with repository identification refactoring.The change from passing the
repoobject torepo.RepoFullNamestring is consistent with the broader refactoring across the codebase to use repository full names instead of repository objects or IDs for project identification.backend/models/scheduler_test.go (1)
57-57: LGTM! Consistent with repository identification refactoring.The change from passing the
repoobject torepo.RepoFullNamestring maintains consistency with the repository identification refactoring pattern applied across the codebase.backend/tasks/runs_test.go (2)
77-77: LGTM! Consistent with repository identification refactoring.The change from passing the
repoobject torepo.RepoFullNamestring follows the established pattern for the repository identification refactoring across the codebase.
143-143: LGTM! Updated to match new function signature.The change from
nilto an empty string""correctly reflects the updated function signature that now expects a string parameter for repository identification instead of a nullable type.
| slog.Error("failed to load digger.yml: %v", "error", err) | ||
| return fmt.Errorf("error loading digger.yml %v", err) | ||
| } | ||
| models.DB.RefreshProjectsFromRepo(strconv.Itoa(int(link.OrganisationId)), *config, repoFullName) |
There was a problem hiding this comment.
Handle the error return value from RefreshProjectsFromRepo.
The static analysis tool correctly identifies that the error return value from models.DB.RefreshProjectsFromRepo is not being checked, which could mask potential issues during project refresh.
- models.DB.RefreshProjectsFromRepo(strconv.Itoa(int(link.OrganisationId)), *config, repoFullName)
- return nil
+ err = models.DB.RefreshProjectsFromRepo(strconv.Itoa(int(link.OrganisationId)), *config, repoFullName)
+ if err != nil {
+ slog.Error("failed to refresh projects from repo", "error", err, "repoFullName", repoFullName)
+ return fmt.Errorf("error refreshing projects: %v", err)
+ }
+ return nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| models.DB.RefreshProjectsFromRepo(strconv.Itoa(int(link.OrganisationId)), *config, repoFullName) | |
| err = models.DB.RefreshProjectsFromRepo(strconv.Itoa(int(link.OrganisationId)), *config, repoFullName) | |
| if err != nil { | |
| slog.Error("failed to refresh projects from repo", "error", err, "repoFullName", repoFullName) | |
| return fmt.Errorf("error refreshing projects: %v", err) | |
| } | |
| return nil |
🧰 Tools
🪛 golangci-lint (1.64.8)
51-51: Error return value of models.DB.RefreshProjectsFromRepo is not checked
(errcheck)
🤖 Prompt for AI Agents
In backend/services/repos.go at line 51, the call to
models.DB.RefreshProjectsFromRepo returns an error that is currently ignored.
Modify the code to capture this error and handle it appropriately, such as
logging the error or returning it up the call stack, to ensure any issues during
project refresh are not missed.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ee/backend/controllers/web.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
ee/backend/controllers/web.go (2)
backend/services/messages.go (2)
AddError(63-66)GetMessages(73-103)backend/models/orgs.go (3)
Project(74-84)Organisation(9-14)Repo(16-27)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (2)
ee/backend/controllers/web.go (2)
141-141: LGTM! Logging updated to use RepoFullName.This change correctly updates the logging to use
project.RepoFullNameinstead of the deprecatedproject.Repo, aligning with the refactoring to use repository full names for identification.
151-151: LGTM! Policy creation updated to use fetched repository.This change correctly uses the fetched
repovariable instead of the deprecatedproject.Repo, maintaining consistency with the new repository identification approach.
| repo, err := models.DB.GetRepoByFullName(middleware.ORGANISATION_ID_KEY, project.RepoFullName) | ||
| if err != nil { | ||
| log.Printf("Failed to fetch repo by name: %v, %v\n", project.RepoFullName, err) | ||
| message := "Failed to create a policy" | ||
| services.AddError(c, message) | ||
| pageContext := services.GetMessages(c) | ||
| c.HTML(http.StatusOK, "policy_add.tmpl", pageContext) | ||
| } |
There was a problem hiding this comment.
Critical: Missing return statement in error handling block.
The error handling block is missing a return statement, which means execution will continue even when the repository fetch fails. This could lead to using an uninitialized repo variable in line 151, causing a runtime panic.
Apply this diff to fix the issue:
repo, err := models.DB.GetRepoByFullName(middleware.ORGANISATION_ID_KEY, project.RepoFullName)
if err != nil {
log.Printf("Failed to fetch repo by name: %v, %v\n", project.RepoFullName, err)
message := "Failed to create a policy"
services.AddError(c, message)
pageContext := services.GetMessages(c)
c.HTML(http.StatusOK, "policy_add.tmpl", pageContext)
+ return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| repo, err := models.DB.GetRepoByFullName(middleware.ORGANISATION_ID_KEY, project.RepoFullName) | |
| if err != nil { | |
| log.Printf("Failed to fetch repo by name: %v, %v\n", project.RepoFullName, err) | |
| message := "Failed to create a policy" | |
| services.AddError(c, message) | |
| pageContext := services.GetMessages(c) | |
| c.HTML(http.StatusOK, "policy_add.tmpl", pageContext) | |
| } | |
| repo, err := models.DB.GetRepoByFullName(middleware.ORGANISATION_ID_KEY, project.RepoFullName) | |
| if err != nil { | |
| log.Printf("Failed to fetch repo by name: %v, %v\n", project.RepoFullName, err) | |
| message := "Failed to create a policy" | |
| services.AddError(c, message) | |
| pageContext := services.GetMessages(c) | |
| c.HTML(http.StatusOK, "policy_add.tmpl", pageContext) | |
| return | |
| } |
🤖 Prompt for AI Agents
In ee/backend/controllers/web.go around lines 143 to 150, the error handling
block after failing to fetch the repository is missing a return statement,
causing the function to continue execution with an uninitialized repo variable.
Add a return statement immediately after rendering the error page to stop
further execution and prevent potential runtime panics.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores