include projects api #2006
include projects api #2006
Conversation
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Controller
participant DB
Client->>API: GET /api/projects/
API->>Controller: ListProjectsApi(c)
Controller->>DB: Find organisation by external_id/source
alt Org not found
Controller-->>Client: 404 Not Found
else Org found
Controller->>DB: Query projects by organisation_id
alt DB error
Controller-->>Client: 500 Error
else Success
Controller->>Client: JSON { result: [projects with directory] }
end
end
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
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 (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Major updates to project management and logging infrastructure, focusing on API enhancements and data model refinements.
- Removed
configuration_yamlcolumn from projects table and added newdirectoryfield for better project path management - Switched logging from
orandin/slog-gormtoimdatngo/slog-gorm/v2with enhanced logging configuration and slow query monitoring - Added new
/api/projectsGET endpoint with improved organization-level project listing - Added comprehensive debug logging with structured slog.Debug across GitHub client utilities
- Improved error handling in projects API with better HTTP status codes and organization access validation
14 files reviewed, 8 comments
Edit PR Review Bot Settings | Greptile
| @@ -0,0 +1,2 @@ | |||
| -- Modify "projects" table | |||
| ALTER TABLE "public"."projects" DROP COLUMN "configuration_yaml"; | |||
There was a problem hiding this comment.
style: Consider using IF EXISTS clause to prevent errors if column was already dropped in some environments
| ALTER TABLE "public"."projects" DROP COLUMN "configuration_yaml"; | |
| ALTER TABLE "public"."projects" DROP COLUMN IF EXISTS "configuration_yaml"; |
| projectsApiGroup := apiGroup.Group("/projects") | ||
| projectsApiGroup.GET("/", controllers.ListProjectsApi) |
There was a problem hiding this comment.
style: Log debug context when handling projects API requests to improve traceability
| "log/slog" | ||
| "strconv" | ||
| "strings" | ||
| ) | ||
|
|
||
| func LoadProjectsFromGithubRepo(gh utils2.GithubClientProvider, installationId string, repoFullName string, repoOwner string, repoName string, cloneUrl string, branch string) error { | ||
| func LoadProjectsFromGithubRepo(gh utils3.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.
syntax: Typo in error log parameter name 'insallationId'
| slog.Error("failed to convert installation id %v to int64", "insallationId", installationId) | |
| slog.Error("failed to convert installation id %v to int64", "installationId", installationId) |
| @@ -140,7 +140,7 @@ func TestThatRunQueueItemMovesFromQueuedToPlanningAfterPickup(t *testing.T) { | |||
| for i, testParam := range testParameters { | |||
| ciService := github2.MockCiService{} | |||
| batch, _ := models.DB.CreateDiggerBatch(models.DiggerVCSGithub, 123, "", "", "", 22, "", "", "", nil, 0, "", false, true, nil) | |||
| project, _ := models.DB.CreateProject(fmt.Sprintf("test%v", i), nil, "", false, false) | |||
| project, _ := models.DB.CreateProject(fmt.Sprintf("test%v", i), "", nil, "", false, false) | |||
There was a problem hiding this comment.
logic: Missing error handling for CreateProject - ignoring error could mask test failures
| project, _ := models.DB.CreateProject(fmt.Sprintf("test%v", i), "", nil, "", false, false) | |
| project, err := models.DB.CreateProject(fmt.Sprintf("test%v", i), "", nil, "", false, false) | |
| assert.NoError(t, err) |
| @@ -20,8 +20,19 @@ var DEFAULT_ORG_NAME = "digger" | |||
| var DB *Database | |||
|
|
|||
| func ConnectDatabase() { | |||
| slogger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelInfo})) | |||
There was a problem hiding this comment.
logic: Set log level based on DIGGER_LOG_LEVEL environment variable instead of hardcoding to LevelInfo
| slogger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelInfo})) | |
| logLevel := slog.LevelInfo | |
| if os.Getenv("DIGGER_LOG_LEVEL") == "DEBUG" { | |
| logLevel = slog.LevelDebug | |
| } | |
| slogger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: logLevel})) |
| net "net/http" | ||
| "os" | ||
| "path" | ||
| "strconv" | ||
| ) | ||
|
|
||
| func GetGithubClient(gh utils.GithubClientProvider, installationId int64, repoFullName string) (*github.Client, *string, error) { | ||
| slog.Debug("Getting GitHub client", | ||
| "installationId", installationId, | ||
| "repoFullName", repoFullName) | ||
| repo, err := dbmodels.DB.GetRepoByInstllationIdAndRepoFullName(strconv.FormatInt(installationId, 10), repoFullName) |
There was a problem hiding this comment.
logic: Method name contains typo: 'Instllation' is missing an 'a'
| repo, err := dbmodels.DB.GetRepoByInstllationIdAndRepoFullName(strconv.FormatInt(installationId, 10), repoFullName) | |
| repo, err := dbmodels.DB.GetRepoByInstallationIdAndRepoFullName(strconv.FormatInt(installationId, 10), repoFullName) |
| "log" | ||
| "log/slog" |
There was a problem hiding this comment.
style: Using both log and slog packages - consider standardizing on slog throughout
| project := models.Project{ | ||
| Name: request.Name, | ||
| ConfigurationYaml: request.ConfigurationYaml, | ||
| OrganisationID: org.ID, | ||
| RepoFullName: repo.RepoFullName, | ||
| Organisation: org, | ||
| Name: request.Name, | ||
| OrganisationID: org.ID, | ||
| RepoFullName: repo.RepoFullName, | ||
| Organisation: org, | ||
| } |
There was a problem hiding this comment.
logic: ConfigurationYaml field has been removed from project creation. Ensure this doesn't break any existing code that expects this field.
|
✅ No security or compliance issues detected. Reviewed everything up to fd3af41. Security Overview
Detected Code Changes
Reply to this PR with |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ee/drift/utils/github.go (1)
38-38: Fix inconsistent capitalization in log message.The log message capitalization is inconsistent with the similar message above (line 24).
- slog.Debug("getting github client", "installationId", installationId, "repoFullName", repoFullName) + slog.Debug("Getting GitHub client", "installationId", installationId, "repoFullName", repoFullName)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
backend/go.sumis excluded by!**/*.sumbackend/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
backend/bootstrap/main.go(1 hunks)backend/controllers/github_test.go(1 hunks)backend/controllers/projects.go(2 hunks)backend/go.mod(1 hunks)backend/migrations/20250704024948.sql(1 hunks)backend/migrations/20250704025208.sql(1 hunks)backend/models/orgs.go(2 hunks)backend/models/scheduler_test.go(1 hunks)backend/models/setup.go(2 hunks)backend/models/storage.go(3 hunks)backend/services/repos.go(2 hunks)backend/tasks/runs_test.go(2 hunks)ee/drift/utils/github.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
backend/bootstrap/main.go (1)
backend/controllers/projects.go (1)
ListProjectsApi(28-72)
ee/drift/utils/github.go (2)
backend/utils/github.go (2)
GetGithubClient(183-196)GithubClientProvider(89-93)next/utils/github.go (2)
GetGithubClient(109-118)GithubClientProvider(44-48)
backend/models/storage.go (3)
backend/models/setup.go (1)
Database(13-15)backend/models/orgs.go (2)
Organisation(9-14)Project(74-84)libs/digger_config/config.go (1)
Project(44-62)
backend/controllers/projects.go (3)
backend/middleware/jwt.go (2)
ORGANISATION_ID_KEY(326-326)ORGANISATION_SOURCE_KEY(327-327)backend/models/orgs.go (2)
Organisation(9-14)Project(74-84)backend/models/setup.go (1)
DB(20-20)
backend/services/repos.go (1)
ee/drift/utils/github.go (1)
GetGithubService(37-52)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Security Check
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (17)
backend/go.mod (1)
23-23: Dependency Verified: slog-gorm updated to v2.0.0
Thegithub.com/imdatngo/slog-gorm/v2module v2.0.0 (released May 7, 2024) exists and has no known security advisories. Please run your integration tests to confirm compatibility with this new major version.backend/migrations/20250704024948.sql (1)
1-2: LGTM! Migration follows best practices.The migration safely adds a nullable column, which won't break existing data. The SQL syntax is correct for PostgreSQL.
ee/drift/utils/github.go (2)
16-16: LGTM! Good addition for observability.Adding structured logging with slog improves debugging capabilities.
24-26: LGTM! Structured logging with relevant parameters.The debug log provides good context for troubleshooting GitHub client issues.
backend/bootstrap/main.go (1)
232-234: LGTM! API route follows established patterns.The new projects API endpoint is properly configured with appropriate middleware and follows the existing routing structure. The controller function
ListProjectsApicorrectly handles the endpoint logic as shown in the relevant code snippets.backend/controllers/github_test.go (1)
617-617: LGTM: Correctly updated CreateProject call for new signature.The addition of an empty string as the second argument properly aligns with the updated
CreateProjectmethod signature that now includes adirectoryparameter.backend/tasks/runs_test.go (1)
77-77: LGTM: CreateProject calls properly updated for new signature.Both instances correctly add the empty string directory parameter to align with the updated
CreateProjectmethod signature.Also applies to: 143-143
backend/models/scheduler_test.go (1)
57-57: LGTM: CreateProject call correctly updated.The addition of the empty string directory parameter properly aligns with the updated method signature.
backend/models/orgs.go (2)
77-77: LGTM: Directory field properly added to Project struct.The new
Directorystring field is correctly positioned and will support the enhanced project structure.
108-108: LGTM: JSON mapping updated to include Directory field.The
MapToJsonStructmethod correctly includes the newDirectoryfield in the JSON output, maintaining consistency with the struct changes.backend/models/setup.go (1)
23-36: LGTM: Database logging upgraded with structured logging.The migration to slog-gorm v2 with proper configuration enhances observability. The logger setup includes appropriate settings for grouping, slow query detection, and conditional debug tracing.
backend/services/repos.go (1)
13-13: LGTM! Package refactoring looks correct.The function signature update to use
utils3.GithubClientProviderand the corresponding change to callutils3.GetGithubServicealong with the added debug logging are all consistent with the package reorganization.Also applies to: 37-38
backend/models/storage.go (3)
381-381: LGTM! Clear logging message improvement.The updated log message clarifies that this method fetches by full name rather than just name.
1405-1408: LGTM! Directory field addition is consistent with schema changes.The new
directoryparameter and its assignment to theDirectoryfield aligns well with the schema migration that adds this field.
1646-1652: LGTM! Directory handling implementation is correct.The extraction of
projectDirectoryfrom the configuration and its use in both project creation and updates is properly implemented and consistent with the schema changes.backend/controllers/projects.go (2)
28-72: LGTM! Well-implemented API endpoint with proper error handling.The new
ListProjectsApifunction demonstrates good practices:
- Proper error handling with appropriate HTTP status codes
- Comprehensive logging for both success and error cases
- Clean separation of concerns between organization lookup and project querying
- Consistent response structure
The implementation correctly relies on middleware validation for organization context and handles all error scenarios appropriately.
397-402: LGTM! ConfigurationYaml field removal is consistent with schema changes.The removal of the
ConfigurationYamlfield assignment aligns with the database schema migration that drops this column and the model changes that remove this field from the Project struct.
| -- Modify "projects" table | ||
| ALTER TABLE "public"."projects" DROP COLUMN "configuration_yaml"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that all references to configuration_yaml have been removed.
This migration drops the configuration_yaml column, which is a destructive operation. Ensure that:
- All code references to
ConfigurationYamlfield have been removed or updated - Any data migration from
configuration_yamltodirectoryhas been completed - This migration is safe to run in production
🏁 Script executed:
#!/bin/bash
# Search for any remaining references to configuration_yaml or ConfigurationYaml
echo "Searching for configuration_yaml references..."
rg -i "configuration_yaml|configurationyaml" --type go --type sql
echo "Searching for ConfigurationYaml field references..."
rg "ConfigurationYaml" --type go -A 3 -B 3Length of output: 9506
Ensure Complete Removal of configuration_yaml References and Safe Data Migration
Please address the following before merging this destructive migration:
- Remove all code references to the dropped
configuration_yamlcolumn and replace them with the newdirectoryfield (or appropriate substitute):next/model/projects.gen.go&next/models_generated/projects.gen.go: eliminate theConfigurationYamlfield, regenerate models.backend/controllers/projects.go: removeConfigurationYamlfromCreateProjectRequestand any handlers (e.g.,ReportProjectsForRepo).libs/backendapi/diggerapi.go: drop theconfigurationYamlparameter and JSON payload, update client calls to usedirectory.
- Add a pre-migration step (in an earlier migration) to copy existing
configuration_yamldata intodirectory(or the new storage) for all rows. - Delete or update the older migration (
20231227132525.sql) that initially createdconfiguration_yaml, if it’s no longer needed. - Regenerate and commit any affected codegen artifacts (models, clients) so the codebase compiles without the old field.
- Verify in a staging environment that:
- Data is correctly migrated.
- No runtime errors occur in endpoints or background jobs.
- The new field behaves as expected.
🤖 Prompt for AI Agents
In backend/migrations/20250704025208.sql at lines 1-2, the migration drops the
configuration_yaml column but does not handle related code or data migration. To
fix this, remove all references to configuration_yaml in the codebase including
the specified Go model files, controller, and API client, replacing them with
the directory field. Add a prior migration that copies data from
configuration_yaml to directory for all existing rows. Also, delete or update
the older migration that created configuration_yaml if obsolete. Regenerate all
affected codegen artifacts to reflect these changes and verify in staging that
data migration is successful and no runtime errors occur.
| } else { | ||
| p.Directory = projectDirectory | ||
| db.GormDB.Save(p) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the database save operation.
The direct save operation on line 1658 lacks error handling, which could lead to silent failures when updating the project directory.
} else {
p.Directory = projectDirectory
- db.GormDB.Save(p)
+ err := db.GormDB.Save(p).Error
+ if err != nil {
+ return fmt.Errorf("error updating project directory: %v", 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.
| } else { | |
| p.Directory = projectDirectory | |
| db.GormDB.Save(p) | |
| } | |
| } else { | |
| p.Directory = projectDirectory | |
| err := db.GormDB.Save(p).Error | |
| if err != nil { | |
| return fmt.Errorf("error updating project directory: %v", err) | |
| } | |
| } |
🤖 Prompt for AI Agents
In backend/models/storage.go around lines 1656 to 1659, the call to
db.GormDB.Save(p) does not handle potential errors, risking silent failures.
Modify the code to capture the error returned by the Save operation, check if an
error occurred, and handle it appropriately, such as logging the error or
returning it to the caller to ensure failures are not ignored.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Database
Tests