feat/drift fields #2018
feat/drift fields #2018
Conversation
WalkthroughThis update introduces new API endpoints for retrieving and updating drift-related settings at both the organization and project levels. It extends the database schema and data models to support drift detection features, including new fields for drift status, scheduling, and webhook integration. Controllers and serialization logic are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Controllers
participant DB
Client->>API: GET /api/orgs/settings/
API->>Controllers: GetOrgSettingsApi
Controllers->>DB: Query organisation by external_id & source
DB-->>Controllers: Organisation record
Controllers-->>API: JSON (drift_enabled, drift_cron_tab, drift_webhook_url)
API-->>Client: Response
Client->>API: PUT /api/orgs/settings/ (JSON body)
API->>Controllers: UpdateOrgSettingsApi
Controllers->>DB: Query & update organisation
DB-->>Controllers: Save result
Controllers-->>API: 200 OK (empty JSON)
API-->>Client: Response
Client->>API: PUT /api/projects/:project_id/ (JSON body)
API->>Controllers: UpdateProjectApi
Controllers->>DB: Query organisation, then project
Controllers->>DB: Update project drift_enabled
DB-->>Controllers: Save result
Controllers-->>API: Updated project JSON
API-->>Client: Response
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
✨ 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
Implements comprehensive drift detection functionality across organizations and projects with configurable settings and status tracking.
- Added project-level drift tracking in
backend/models/orgs.gowith DriftStatus enum (new/no/acknowledged) and plan storage - Introduced organization-wide drift settings in
backend/controllers/orgs.gowith webhook URLs and cron scheduling - Added migrations (20250710234046.sql, 20250711030148.sql) for drift configuration storage with appropriate defaults
- Implemented new API endpoints in
backend/bootstrap/main.gofor managing drift settings at both org and project levels
9 files reviewed, 10 comments
Edit PR Review Bot Settings | Greptile
backend/controllers/orgs.go
Outdated
| @@ -17,6 +21,72 @@ type TenantCreatedEvent struct { | |||
| Name string `json:"name,omitempty"` | |||
| } | |||
|
|
|||
| func GetOrgSettingsApi(c *gin.Context) { | |||
| // TODO | |||
There was a problem hiding this comment.
style: Remove TODO comment - implementation is complete
| // TODO | |
| func GetOrgSettingsApi(c *gin.Context) { |
| @@ -0,0 +1,2 @@ | |||
| -- Modify "organisations" table | |||
| ALTER TABLE "public"."organisations" ADD COLUMN "drift_enabled" boolean NULL; | |||
There was a problem hiding this comment.
style: Consider setting a default value of FALSE for drift_enabled to ensure explicit opt-in for drift detection
| @@ -0,0 +1,2 @@ | |||
| -- Modify "organisations" table | |||
| ALTER TABLE "public"."organisations" ADD COLUMN "drift_enabled" boolean NULL; | |||
There was a problem hiding this comment.
logic: This column should not be nullable since it represents a feature flag. Boolean columns should typically have a definite true/false value
| DriftCronTab string `json:"drift_cron_tab"` | ||
| DriftWebhookUrl string `json:"drift_webhook_url"` | ||
| } | ||
| err = json.NewDecoder(c.Request.Body).Decode(&reqBody) |
There was a problem hiding this comment.
style: Consider using c.ShouldBindJSON() for consistency with other endpoints in the codebase
backend/controllers/orgs.go
Outdated
| } | ||
|
|
||
| func UpdateOrgSettingsApi(c *gin.Context) { | ||
| // TODO |
There was a problem hiding this comment.
style: Remove TODO comment - implementation is complete
| // TODO | |
| func UpdateOrgSettingsApi(c *gin.Context) { |
| @@ -0,0 +1,2 @@ | |||
| -- Modify "projects" table | |||
| ALTER TABLE "public"."projects" ADD COLUMN "drift_enabled" boolean NULL DEFAULT false, ADD COLUMN "drift_status" text NULL DEFAULT 'no drift', ADD COLUMN "latest_drift_check" timestamptz NULL, ADD COLUMN "drift_terraform_plan" text NULL; | |||
There was a problem hiding this comment.
style: Consider adding CHECK constraint to limit drift_status to specific valid values (e.g., 'no drift', 'drift detected', 'checking')
| @@ -0,0 +1,2 @@ | |||
| -- Modify "projects" table | |||
| ALTER TABLE "public"."projects" ADD COLUMN "drift_enabled" boolean NULL DEFAULT false, ADD COLUMN "drift_status" text NULL DEFAULT 'no drift', ADD COLUMN "latest_drift_check" timestamptz NULL, ADD COLUMN "drift_terraform_plan" text NULL; | |||
There was a problem hiding this comment.
style: The drift-related fields currently allow NULL. Add NOT NULL constraints for critical fields like drift_enabled and drift_status since they have defaults
| ALTER TABLE "public"."projects" ADD COLUMN "drift_enabled" boolean NULL DEFAULT false, ADD COLUMN "drift_status" text NULL DEFAULT 'no drift', ADD COLUMN "latest_drift_check" timestamptz NULL, ADD COLUMN "drift_terraform_plan" text NULL; | |
| ALTER TABLE "public"."projects" ADD COLUMN "drift_enabled" boolean NOT NULL DEFAULT false, ADD COLUMN "drift_status" text NOT NULL DEFAULT 'no drift', ADD COLUMN "latest_drift_check" timestamptz NULL, ADD COLUMN "drift_terraform_plan" text NULL; |
backend/models/orgs.go
Outdated
| type DriftStatus string | ||
|
|
||
| var DriftStatusNewDrift = "new drift" | ||
| var DriftStatusNoDrift = "no drift" | ||
| var DriftStatusAcknowledgeDrift = "acknowledged drift" |
There was a problem hiding this comment.
style: DriftStatus constants should use const declarations instead of var for type safety and immutability.
| type DriftStatus string | |
| var DriftStatusNewDrift = "new drift" | |
| var DriftStatusNoDrift = "no drift" | |
| var DriftStatusAcknowledgeDrift = "acknowledged drift" | |
| type DriftStatus string | |
| const ( | |
| DriftStatusNewDrift DriftStatus = "new drift" | |
| DriftStatusNoDrift DriftStatus = "no drift" | |
| DriftStatusAcknowledgeDrift DriftStatus = "acknowledged drift" | |
| ) |
| @@ -109,6 +126,10 @@ func (p *Project) MapToJsonStruct() interface{} { | |||
| OrganisationID: p.OrganisationID, | |||
| OrganisationName: p.Organisation.Name, | |||
| RepoFullName: p.RepoFullName, | |||
| DriftEnabled: p.DriftEnabled, | |||
| DriftStatus: string(p.DriftStatus), | |||
| LatestDriftCheck: p.LatestDriftCheck.String(), | |||
There was a problem hiding this comment.
logic: Direct String() call on time.Time may produce inconsistent formats. Consider using a standardized time format.
backend/controllers/projects.go
Outdated
| err = models.DB.GormDB.Save(&project).Error | ||
| if err != nil { | ||
| slog.Error("Error updating project", "organisationId", organisationId, "orgId", org.ID, "error", err) | ||
| c.String(http.StatusInternalServerError, "Unknown error occurred while fetching database") |
There was a problem hiding this comment.
logic: Error message suggests fetching database when it's actually an error updating
| c.String(http.StatusInternalServerError, "Unknown error occurred while fetching database") | |
| c.String(http.StatusInternalServerError, "Unknown error occurred while updating database") |
|
✅ No security or compliance issues detected. Reviewed everything up to 8d30649. 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 (5)
backend/controllers/projects.go (2)
92-100: Consider adding input validation for the request body.While JSON decoding error handling is present, consider adding validation to ensure the request body contains expected fields and values.
var reqBody struct { DriftEnabled bool `json:"drift_enabled"` } err = json.NewDecoder(c.Request.Body).Decode(&reqBody) if err != nil { slog.Error("Error decoding request body", "error", err) c.String(http.StatusBadRequest, "Error decoding request body") return } + +// Optional: Add validation if needed +// if reqBody.DriftEnabled != true && reqBody.DriftEnabled != false { +// c.String(http.StatusBadRequest, "Invalid drift_enabled value") +// return +// }
123-123: Consider security implications of returning the full project object.The function returns the entire project object, which might expose sensitive information. Consider returning only the updated fields or a success message.
-c.JSON(http.StatusOK, project) +c.JSON(http.StatusOK, gin.H{ + "drift_enabled": project.DriftEnabled, + "message": "Project updated successfully", +})backend/controllers/orgs.go (2)
24-47: Address the TODO comment and consider input validation.The function structure is solid with proper error handling and logging. However, the TODO comment should be addressed or removed.
-// TODO +// Get organisation drift settingsDo you want me to help implement any specific functionality mentioned in the TODO, or should this comment be removed?
50-50: Remove or address the TODO comment.The TODO comment should be replaced with a proper description or removed if no additional work is needed.
-// TODO +// Update organisation drift settingsbackend/models/orgs.go (1)
115-132: Consider using ISO 8601 format for timestamp serialization.The drift fields are properly included in the JSON mapping. However, consider using a more standard time format for API consumers.
DriftEnabled bool `json:"drift_enabled"` DriftStatus string `json:"drift_status"` - LatestDriftCheck string `json:"latest_drift_check"` + LatestDriftCheck string `json:"latest_drift_check"` DriftTerraformPlan string `json:"drift_terraform_plan"`And in the struct initialization:
DriftEnabled: p.DriftEnabled, DriftStatus: string(p.DriftStatus), - LatestDriftCheck: p.LatestDriftCheck.String(), + LatestDriftCheck: p.LatestDriftCheck.Format(time.RFC3339), DriftTerraformPlan: p.DriftTerraformPlan,This provides a more standard ISO 8601 format that's easier to parse by API consumers.
📜 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 (8)
backend/bootstrap/main.go(1 hunks)backend/controllers/orgs.go(2 hunks)backend/controllers/projects.go(1 hunks)backend/migrations/20250710234046.sql(1 hunks)backend/migrations/20250711030148.sql(1 hunks)backend/migrations/20250711030248.sql(1 hunks)backend/migrations/20250711030323.sql(1 hunks)backend/models/orgs.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
backend/controllers/projects.go (1)
Learnt from: motatoes
PR: diggerhq/digger#1864
File: backend/controllers/projects.go:672-678
Timestamp: 2024-12-30T09:19:58.162Z
Learning: In file `backend/controllers/projects.go`, the user clarified that the AI summary token is considered non-critical, so they prefer not to validate it.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- 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
🔇 Additional comments (9)
backend/migrations/20250710234046.sql (1)
1-2: Migration looks good with proper defaults and data types.The migration correctly adds drift-related fields to the projects table with sensible defaults and appropriate data types. The nullable fields ensure backwards compatibility.
backend/migrations/20250711030323.sql (1)
1-2: Migration dependency order verifiedThe prior migration
20250711030248.sqlis present and will run before20250711030323.sql, so thedrift_enabledcolumn exists when this default is applied. No further action needed.backend/migrations/20250711030248.sql (1)
1-2: Clean migration adding drift_enabled column.The migration properly adds the
drift_enabledboolean column as nullable, maintaining backwards compatibility. The default value is appropriately set in a subsequent migration.backend/migrations/20250711030148.sql (1)
1-2: Migration correctly adds webhook and cron scheduling fields.The migration properly adds drift configuration fields with appropriate defaults. The cron expression
'0 0 * * *'(daily at midnight) is a sensible default for drift checking.backend/bootstrap/main.go (2)
238-238: Approve addition of PUT /:project_id/ endpointThe new
PUT /:project_id/route is consistent with existing API patterns, and theUpdateProjectApihandler is correctly implemented inbackend/controllers/projects.go. No further changes required.
228-231: Controller implementations confirmedBoth
GetOrgSettingsApiandUpdateOrgSettingsApiare defined and implemented inbackend/controllers/orgs.go(lines 24–47 and 49–89). No further changes needed—approving these route additions.backend/controllers/projects.go (1)
74-124: Well-structured API handler with good error handling.The function follows established patterns in the codebase and includes proper error handling, logging, and middleware integration. The implementation is clean and consistent.
backend/models/orgs.go (2)
14-16: Well-structured drift fields with appropriate GORM tags.The new drift-related fields are properly defined with sensible defaults and follow GORM conventions.
90-93: Well-integrated drift fields with proper GORM configuration.The new drift-related fields are properly integrated into the Project struct with appropriate defaults and GORM tags.
backend/controllers/orgs.go
Outdated
| func UpdateOrgSettingsApi(c *gin.Context) { | ||
| // TODO | ||
| organisationId := c.GetString(middleware.ORGANISATION_ID_KEY) | ||
| organisationSource := c.GetString(middleware.ORGANISATION_SOURCE_KEY) | ||
|
|
||
| var org models.Organisation | ||
| err := models.DB.GormDB.Where("external_id = ? AND external_source = ?", organisationId, organisationSource).First(&org).Error | ||
| if err != nil { | ||
| if errors.Is(err, gorm.ErrRecordNotFound) { | ||
| slog.Info("Organisation not found", "organisationId", organisationId, "source", organisationSource) | ||
| c.String(http.StatusNotFound, "Could not find organisation: "+organisationId) | ||
| } else { | ||
| slog.Error("Error fetching organisation", "organisationId", organisationId, "source", organisationSource, "error", err) | ||
| c.String(http.StatusInternalServerError, "Error fetching organisation") | ||
| } | ||
| return | ||
| } | ||
| var reqBody struct { | ||
| DriftEnabled bool `json:"drift_enabled"` | ||
| DriftCronTab string `json:"drift_cron_tab"` | ||
| DriftWebhookUrl string `json:"drift_webhook_url"` | ||
| } | ||
| err = json.NewDecoder(c.Request.Body).Decode(&reqBody) | ||
| if err != nil { | ||
| slog.Error("Error decoding request body", "error", err) | ||
| c.String(http.StatusBadRequest, "Error decoding request body") | ||
| return | ||
| } | ||
|
|
||
| org.DriftEnabled = reqBody.DriftEnabled | ||
| org.DriftCronTab = reqBody.DriftCronTab | ||
| org.DriftWebhookUrl = reqBody.DriftWebhookUrl | ||
| err = models.DB.GormDB.Save(&org).Error | ||
| if err != nil { | ||
| slog.Error("Error saving organisation", "organisationId", organisationId, "source", organisationSource, "error", err) | ||
| c.String(http.StatusInternalServerError, "Error saving organisation") | ||
| return | ||
| } | ||
|
|
||
| c.JSON(http.StatusOK, gin.H{}) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address TODO comment and add input validation for cron expression and webhook URL.
The function structure is good, but consider adding validation for the cron expression format and webhook URL.
+import (
+ "net/url"
+ "regexp"
+)
var reqBody struct {
DriftEnabled bool `json:"drift_enabled"`
DriftCronTab string `json:"drift_cron_tab"`
DriftWebhookUrl string `json:"drift_webhook_url"`
}
err = json.NewDecoder(c.Request.Body).Decode(&reqBody)
if err != nil {
slog.Error("Error decoding request body", "error", err)
c.String(http.StatusBadRequest, "Error decoding request body")
return
}
+// Validate cron expression format (basic validation)
+if reqBody.DriftCronTab != "" {
+ cronRegex := regexp.MustCompile(`^(\*|[0-9]+|\*/[0-9]+|[0-9]+-[0-9]+) (\*|[0-9]+|\*/[0-9]+|[0-9]+-[0-9]+) (\*|[0-9]+|\*/[0-9]+|[0-9]+-[0-9]+) (\*|[0-9]+|\*/[0-9]+|[0-9]+-[0-9]+) (\*|[0-9]+|\*/[0-9]+|[0-9]+-[0-9]+)$`)
+ if !cronRegex.MatchString(reqBody.DriftCronTab) {
+ c.String(http.StatusBadRequest, "Invalid cron expression format")
+ return
+ }
+}
+
+// Validate webhook URL format
+if reqBody.DriftWebhookUrl != "" {
+ if _, err := url.Parse(reqBody.DriftWebhookUrl); err != nil {
+ c.String(http.StatusBadRequest, "Invalid webhook URL format")
+ 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.
| func UpdateOrgSettingsApi(c *gin.Context) { | |
| // TODO | |
| organisationId := c.GetString(middleware.ORGANISATION_ID_KEY) | |
| organisationSource := c.GetString(middleware.ORGANISATION_SOURCE_KEY) | |
| var org models.Organisation | |
| err := models.DB.GormDB.Where("external_id = ? AND external_source = ?", organisationId, organisationSource).First(&org).Error | |
| if err != nil { | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| slog.Info("Organisation not found", "organisationId", organisationId, "source", organisationSource) | |
| c.String(http.StatusNotFound, "Could not find organisation: "+organisationId) | |
| } else { | |
| slog.Error("Error fetching organisation", "organisationId", organisationId, "source", organisationSource, "error", err) | |
| c.String(http.StatusInternalServerError, "Error fetching organisation") | |
| } | |
| return | |
| } | |
| var reqBody struct { | |
| DriftEnabled bool `json:"drift_enabled"` | |
| DriftCronTab string `json:"drift_cron_tab"` | |
| DriftWebhookUrl string `json:"drift_webhook_url"` | |
| } | |
| err = json.NewDecoder(c.Request.Body).Decode(&reqBody) | |
| if err != nil { | |
| slog.Error("Error decoding request body", "error", err) | |
| c.String(http.StatusBadRequest, "Error decoding request body") | |
| return | |
| } | |
| org.DriftEnabled = reqBody.DriftEnabled | |
| org.DriftCronTab = reqBody.DriftCronTab | |
| org.DriftWebhookUrl = reqBody.DriftWebhookUrl | |
| err = models.DB.GormDB.Save(&org).Error | |
| if err != nil { | |
| slog.Error("Error saving organisation", "organisationId", organisationId, "source", organisationSource, "error", err) | |
| c.String(http.StatusInternalServerError, "Error saving organisation") | |
| return | |
| } | |
| c.JSON(http.StatusOK, gin.H{}) | |
| } | |
| // Make sure to import "net/url" and "regexp" at the top of the file. | |
| func UpdateOrgSettingsApi(c *gin.Context) { | |
| // TODO | |
| organisationId := c.GetString(middleware.ORGANISATION_ID_KEY) | |
| organisationSource := c.GetString(middleware.ORGANISATION_SOURCE_KEY) | |
| var org models.Organisation | |
| err := models.DB.GormDB. | |
| Where("external_id = ? AND external_source = ?", organisationId, organisationSource). | |
| First(&org). | |
| Error | |
| if err != nil { | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| slog.Info("Organisation not found", "organisationId", organisationId, "source", organisationSource) | |
| c.String(http.StatusNotFound, "Could not find organisation: "+organisationId) | |
| } else { | |
| slog.Error("Error fetching organisation", "organisationId", organisationId, "source", organisationSource, "error", err) | |
| c.String(http.StatusInternalServerError, "Error fetching organisation") | |
| } | |
| return | |
| } | |
| var reqBody struct { | |
| DriftEnabled bool `json:"drift_enabled"` | |
| DriftCronTab string `json:"drift_cron_tab"` | |
| DriftWebhookUrl string `json:"drift_webhook_url"` | |
| } | |
| err = json.NewDecoder(c.Request.Body).Decode(&reqBody) | |
| if err != nil { | |
| slog.Error("Error decoding request body", "error", err) | |
| c.String(http.StatusBadRequest, "Error decoding request body") | |
| return | |
| } | |
| // Validate cron expression format (basic validation) | |
| if reqBody.DriftCronTab != "" { | |
| cronRegex := regexp.MustCompile(`^(\*|[0-9]+|\*/[0-9]+|[0-9]+-[0-9]+) (\*|[0-9]+|\*/[0-9]+|[0-9]+-[0-9]+) (\*|[0-9]+|\*/[0-9]+|[0-9]+-[0-9]+) (\*|[0-9]+|\*/[0-9]+|[0-9]+-[0-9]+) (\*|[0-9]+|\*/[0-9]+|[0-9]+-[0-9]+)$`) | |
| if !cronRegex.MatchString(reqBody.DriftCronTab) { | |
| c.String(http.StatusBadRequest, "Invalid cron expression format") | |
| return | |
| } | |
| } | |
| // Validate webhook URL format | |
| if reqBody.DriftWebhookUrl != "" { | |
| if _, err := url.Parse(reqBody.DriftWebhookUrl); err != nil { | |
| c.String(http.StatusBadRequest, "Invalid webhook URL format") | |
| return | |
| } | |
| } | |
| org.DriftEnabled = reqBody.DriftEnabled | |
| org.DriftCronTab = reqBody.DriftCronTab | |
| org.DriftWebhookUrl = reqBody.DriftWebhookUrl | |
| err = models.DB.GormDB.Save(&org).Error | |
| if err != nil { | |
| slog.Error("Error saving organisation", "organisationId", organisationId, "source", organisationSource, "error", err) | |
| c.String(http.StatusInternalServerError, "Error saving organisation") | |
| return | |
| } | |
| c.JSON(http.StatusOK, gin.H{}) | |
| } |
🤖 Prompt for AI Agents
In backend/controllers/orgs.go around lines 49 to 89, the UpdateOrgSettingsApi
function lacks validation for the cron expression and webhook URL inputs. Add
validation logic after decoding the request body to verify that DriftCronTab is
a valid cron expression and DriftWebhookUrl is a properly formatted URL. If
validation fails, respond with an appropriate HTTP 400 error and message. This
ensures only valid data is saved to the organisation record.
Summary by CodeRabbit
New Features
Database Changes