publish ai summaries #1864
publish ai summaries #1864
Conversation
WalkthroughThis pull request introduces enhancements to Digger's functionality by adding AI-generated summary capabilities for GitHub interactions and Terraform plans. The changes span multiple backend files, introducing new methods, parameters, and data structures to support AI-powered reporting and summary generation. Key modifications allow for the generation of AI summaries for Terraform plans and their integration into GitHub comments, improving user interaction and feedback. Changes
Suggested Reviewers
Possibly Related PRs
Poem
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: 0
🧹 Nitpick comments (14)
backend/controllers/projects.go (1)
491-496: Consider returning after sending the HTTP 500 response.
Here, the code logs the error and sends an HTTP 500 back to the client, but then continues executing the remaining statements in this function. If the summary is required for the subsequent steps, you might want to return early to avoid inconsistent states or unexpected behavior.err = CreateTerraformPlansSummary(d.GithubClientProvider, batch) if err != nil { log.Printf("could not generate terraform plans summary: %v", err) c.JSON(http.StatusInternalServerError, gin.H{"error": "could not generate terraform plans summary"}) + return }backend/utils/ai.go (1)
71-131: Correct the error message to reflect summary generation instead of code generation.
The function's error message references "generating code" instead of "generating summary," which could be confusing to maintainers.- return "", fmt.Errorf("unexpected error occured while generating code") + return "", fmt.Errorf("unexpected error occurred while generating summary")backend/models/scheduler.go (1)
25-39: Clarify usage of AiSummaryCommentId and ReportTerraformOutputs fields.
IfAiSummaryCommentIdcan be empty, consider using a pointer (*string) or clearly handle an empty string. Similarly, ensure thatReportTerraformOutputsis well-documented for how it impacts the batch lifecycle.backend/services/spec.go (1)
119-121: Ensure naming consistency between field and batch property.
Here, the code setsReportTerraformOutputusingbatch.ReportTerraformOutputs. Consider aligning these two property names (singular vs. plural) to avoid confusion.backend/models/storage_test.go (1)
154-154: Expand test coverage for non-empty aiSummaryCommentId usage.
You always pass an empty string foraiSummaryCommentId. Adding a test scenario with a non-empty string (andtrueforreportTerraformOutputs) can improve confidence in the feature.backend/utils/graphs.go (1)
46-46: Check for error handling improvements.
When creating the digger batch, the code logs an error if creation fails but doesn’t attempt any fallback or rollback. A more robust approach might include gracefully aborting or cleaning up resources if batch creation fails.ee/backend/hooks/github.go (1)
154-154: Clarify usage of AI summary parameters.
Here,_is returned for themap[string]*models.DiggerJobinstead of storing it in a variable. Consider whether the AI summary-related flags or comment ID might be relevant to further logic. If so, ensure they’re properly delivered downstream.libs/digger_config/converters.go (1)
50-58: Validate OIDC parameters for AWS Cognito.
The new logic sets fields forAwsCognitoOidcConfig, but no validation is performed. Malformed or missing AWS account data could lead to runtime issues. Consider explicit validation or fallback defaults.libs/digger_config/yaml.go (1)
26-27: Ensure comprehensive test coverage for the newReportingConfigYaml
TheReportingConfigYamlstruct adds valuable AI summary capabilities. Consider adding unit tests validating different configurations (e.g.,AiSummaryset to true/false) to ensure correctness.backend/controllers/github_test.go (2)
734-734: Request improved testing strategy
This line includes a newfalseargument. Recommend adding or extending test assertions confirming that AI summary is intentionally disabled in these scenarios.
796-796: Maintain consistent coverage
Same addition offalse. Confirm that existing tests capture any side effects when AI summary is disabled.backend/models/storage.go (1)
623-637: Validate default persistence behavior for new fields
TheDiggerBatchnow contains two new fields (AiSummaryCommentIdandReportTerraformOutputs). Confirm that the database layer correctly handles their default states (empty string and false). Also, ensure that these fields are retrieved accurately when reading from the database.- AiSummaryCommentId string - ReportTerraformOutputs bool + AiSummaryCommentId string `gorm:"column:ai_summary_comment_id;default:''"` + ReportTerraformOutputs bool `gorm:"column:report_terraform_outputs;default:false"`backend/controllers/github.go (2)
514-525: Consider guarding againstaiSummaryCommentbeingnilafterPublishCommentfailure
Even though the error is logged, there's a non-zero chanceaiSummaryCommentremainsnilifPublishCommentfails. To avoid potential nil-pointer references, consider an early return or a conditional check prior to assigning its ID.var aiSummaryCommentId = "" if config.Reporting.AiSummary { aiSummaryComment, err := ghService.PublishComment(prNumber, "AI Summary will be posted here after completion") if err != nil { log.Printf("could not post ai summary comment: %v", err) commentReporterManager.UpdateComment(fmt.Sprintf(":x: could not post ai comment summary comment id: %v", err)) + return fmt.Errorf("failed to publish AI summary comment: %v", err) } aiSummaryCommentId = aiSummaryComment.Id }
916-926: Avoid duplication by factoring out AI summary comment logic
This AI summary handling code is largely identical to the one used in the pull request event handler. Consider extracting it into a helper function to adhere to DRY principles.var aiSummaryCommentId = "" if config.Reporting.AiSummary { aiSummaryComment, err := ghService.PublishComment(issueNumber, "AI Summary will be posted here after completion") if err != nil { log.Printf("could not post ai summary comment: %v", err) commentReporterManager.UpdateComment(fmt.Sprintf(":x: could not post ai comment summary comment id: %v", err)) + return fmt.Errorf("failed to publish AI summary comment: %v", err) } aiSummaryCommentId = aiSummaryComment.Id }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/controllers/github.go(2 hunks)backend/controllers/github_test.go(4 hunks)backend/controllers/projects.go(3 hunks)backend/models/scheduler.go(1 hunks)backend/models/storage.go(1 hunks)backend/models/storage_test.go(1 hunks)backend/services/spec.go(1 hunks)backend/tasks/runs_test.go(1 hunks)backend/utils/ai.go(2 hunks)backend/utils/graphs.go(2 hunks)ee/backend/controllers/gitlab.go(2 hunks)ee/backend/hooks/github.go(1 hunks)libs/digger_config/config.go(2 hunks)libs/digger_config/converters.go(3 hunks)libs/digger_config/yaml.go(1 hunks)
🔇 Additional comments (15)
backend/controllers/projects.go (1)
657-709:
Fix inconsistent usage of AiSummaryCommentId as a string vs. a nil check.
AiSummaryCommentId is declared as a string in DiggerBatch, but this code checks if batch.AiSummaryCommentId == nil. This will not compile as expected unless the field is declared as a pointer type or you check for "" as the empty value. To avoid runtime errors and confusion, unify the type and usage.
-if batch.AiSummaryCommentId == nil {
+if batch.AiSummaryCommentId == "" {Likely invalid or redundant comment.
backend/utils/ai.go (1)
8-8: New import looks fine.
The log package is commonly used for error logging and debugging.
backend/tasks/runs_test.go (1)
146-146: Consider verifying the new boolean parameter in tests.
Here, you’re passing false for the new reportTerraformOutputs parameter without explicitly checking its effects in the subsequent test flow. It may be helpful to add an assertion verifying that the new behavior (linked to this parameter) meets expectations.
backend/utils/graphs.go (1)
17-17: Validate parameter passing for AI summary.
The expanded function signature adds aiSummaryCommentId string and reportTerraformOutput bool. Ensure all call sites provide meaningful values (or defaults) and handle nil/empty string gracefully.
libs/digger_config/converters.go (2)
172-183: Keep ReporterConfig extensible.
The new copyReporterConfig function is straightforward, returning AiSummary: false when r == nil. If other fields are added later, ensure this function is extended to handle them, avoiding silent misconfigurations.
198-205: Confirm default values for Terraform outputs & reporting.
Setting ReportTerraformOutputs to false by default aligns with a safe default. However, ensure that this default is desirable for existing use cases where there’s an expectation or need to output Terraform results.
libs/digger_config/yaml.go (1)
22-24: Add default values or usage notes for new fields
Great addition to expand reporting capabilities. However, consider clarifying the default behavior or usage instructions for ReportTerraformOutputs in the code comments or documentation, e.g., how the system behaves when this field is nil.
ee/backend/controllers/gitlab.go (2)
336-336: Validate additional parameters for ConvertJobsToDiggerJobs
A new empty string and a hardcoded boolean false have been added to the method call. Verify whether these arguments align with the intended usage (e.g., AI summary comment ID and reportTerraformOutputs), and confirm that calling code fully supports these parameters.
527-527: Retain consistency across different event handlers
The same parameter additions appear here. If there are any future changes to these extra parameters, ensure the same changes are consistently applied to both pull request and issue comment event handlers.
backend/controllers/github_test.go (2)
763-763: Consider negative test cases
Where you pass false to ConvertJobsToDiggerJobs, it might be beneficial to have a test case that ensures no AI summary logic runs when reportTerraformOutputs=false.
841-841: Ensure multiple-level dependency scenarios are tested
This ensures that passing false to ConvertJobsToDiggerJobs does not accidentally skip or otherwise disrupt job creation in a complex project dependency graph.
backend/models/storage.go (1)
620-620: Ensure seamless transition to the new method signature
The new parameters aiSummaryCommentId (string) and reportTerraformOutputs (bool) might break existing calls if they are not updated everywhere in the codebase. Verify that other callers of CreateDiggerBatch have been updated accordingly.
libs/digger_config/config.go (3)
18-20: The new fields introduce valuable reporting functionality
Adding Reporting and ReportTerraformOutputs extends flexibility for advanced AI summary features. Make sure they’re well-documented in relevant docs or docstrings.
22-23: Good separation of responsibilities
Defining ReporterConfig in a separate struct clarifies the AI summary switch, making the code more extensible for future reporting features.
37-52: Field reformatting is acceptable
There are no functional changes, only alignment/formatting adjustments. These changes are fine, but ensure consistency with your team’s style guidelines.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/controllers/projects.go (3)
667-667: Simplify boolean comparisonThe boolean comparison can be simplified.
-if batch.Status == orchestrator_scheduler.BatchJobSucceeded && config.Reporting.AiSummary == true { +if batch.Status == orchestrator_scheduler.BatchJobSucceeded && config.Reporting.AiSummary {🧰 Tools
🪛 golangci-lint (1.62.2)
667-667: S1002: should omit comparison to bool constant, can be simplified to
config.Reporting.AiSummary(gosimple)
681-692: Optimize terraform outputs concatenationThe current string concatenation in a loop could be inefficient. Consider using strings.Builder for better performance.
jobs, err := models.DB.GetDiggerJobsForBatch(batch.ID) -var terraformOutputs = "" +var terraformOutputs strings.Builder for _, job := range jobs { var jobSpec orchestrator_scheduler.JobJson err := json.Unmarshal(job.SerializedJobSpec, &jobSpec) if err != nil { log.Printf("could not summarise plans due to unmarshalling error: %v", err) return fmt.Errorf("could not summarise plans due to unmarshalling error: %v", err) } projectName := jobSpec.ProjectName - terraformOutputs += fmt.Sprintf("terraform output for %v \n\n", projectName) + job.TerraformOutput + fmt.Fprintf(&terraformOutputs, "terraform output for %v \n\n%s", projectName, job.TerraformOutput) }
657-707: Consider splitting the function for better maintainabilityThe function handles multiple responsibilities including config loading, job processing, and API interaction. Consider splitting it into smaller, focused functions:
- Config validation
- Job processing
- AI summary generation
- Comment updating
This would improve testability and make the code more maintainable. Would you like me to provide a refactored version that splits these responsibilities?
🧰 Tools
🪛 golangci-lint (1.62.2)
667-667: S1002: should omit comparison to bool constant, can be simplified to
config.Reporting.AiSummary(gosimple)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ee/backend/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
backend/controllers/projects.go(3 hunks)ee/backend/go.mod(7 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
backend/controllers/projects.go
667-667: S1002: should omit comparison to bool constant, can be simplified to config.Reporting.AiSummary
(gosimple)
🔇 Additional comments (3)
ee/backend/go.mod (3)
20-20: Verify compatibility with the updated lo package.
The github.com/samber/lo package has been updated from v1.39.0 to v1.46.0, which is a significant jump. Ensure all usage patterns are compatible with the new version.
✅ Verification successful
The lo package update appears safe to proceed
Based on the usage patterns found in the codebase, all the functions being used from the lo package are stable and haven't undergone breaking changes between v1.39.0 and v1.46.0. The codebase primarily uses these stable functions:
- Map, FilterMap, Filter: For collection transformations
- Contains, EveryBy: For collection predicates
- Reduce: For collection reduction
- Find: For element finding
- GroupBy, Uniq: For collection organization
- Assign: For map operations
These are core functions that have remained consistent across versions, and reviewing the changelog between these versions shows no breaking changes affecting these functions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for lo package usage patterns that might need updates
rg --type go "lo\." -A 3
Length of output: 7507
303-309: Review the new GORM-related changes.
Several GORM-related packages have been added or updated:
- New: gorm.io/gen v0.3.26
- Updated: gorm.io/driver/postgres v1.5.9
- Updated: gorm.io/gorm v1.25.12
These changes suggest database schema modifications or new ORM features. Ensure that:
- Database migrations are in place
- The gen package is properly configured
- Any new database features are documented
✅ Verification successful
Let me gather more information about the GORM configuration and recent database changes.
GORM changes are properly configured with necessary migrations in place
The verification shows that:
- Database migrations are present and actively maintained (latest migration
20240403155357_drop_dup_idx.sqlhandles index cleanup) - GORM is properly configured in both main and drift services with appropriate database connections and logging
- The
gorm.io/genpackage is extensively used for model generation across the codebase, with generated files present in bothnext/models_generated/andee/drift/models_generated/directories
The changes appear to be well-integrated into the existing infrastructure with proper database maintenance practices in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for GORM-related configurations and migrations
fd -e go -e sql . | xargs rg -l "gorm"
# Look for database migration files
fd -e sql -e go . | xargs rg -l "migrate|migration"
Length of output: 5885
Script:
#!/bin/bash
# Check the migration file content
cat backend/migrations/20240403155357_drop_dup_idx.sql
# Look for GORM configuration in setup files
rg -A 5 "gorm.Open|gorm.Config" backend/models/setup.go ee/drift/dbmodels/setup.go
# Check for gen package usage
rg -l "gorm.io/gen" --type go
Length of output: 5777
14-14: Verify the stability of the drift package.
The newly added github.com/diggerhq/digger/ee/drift package is using a commit hash version (v0.0.0-20241227171610-34d1db1e4299). While this is valid for development, consider using a released version when available for better stability.
✅ Verification successful
Using internal drift package is appropriate
The github.com/diggerhq/digger/ee/drift is an internal package within the same repository, located at ee/drift/. This is a common pattern in Go monorepos where different modules reference each other using commit hashes. The usage of a commit hash version is appropriate in this case as it ensures consistent versioning across the monorepo's modules.
Key findings:
- The drift package is an internal module located at
ee/drift/ - It's extensively used across the codebase with dedicated controllers, models, and services
- Being part of the monorepo, using commit hash versioning is the recommended approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if a released version of the drift package exists
gh api repos/diggerhq/digger/releases | jq -r '.[].tag_name' | grep -i "drift"
Length of output: 78
Script:
#!/bin/bash
# Let's check if the drift package is a submodule or internal package
fd drift -t d
# Also check for any references to drift in the codebase
rg "drift" -g "!{*.mod,*.sum,*.lock}" -l
Length of output: 2141
backend/controllers/projects.go
Outdated
| err = CreateTerraformPlansSummary(d.GithubClientProvider, batch) | ||
| if err != nil { | ||
| log.Printf("could not generate terraform plans summary: %v", err) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "could not generate terraform plans summary"}) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for CreateTerraformPlansSummary
The current implementation continues execution after logging the error and sending a 500 response. This could lead to multiple status codes being sent if subsequent operations fail. Consider handling the error more gracefully:
err = CreateTerraformPlansSummary(d.GithubClientProvider, batch)
if err != nil {
log.Printf("could not generate terraform plans summary: %v", err)
- c.JSON(http.StatusInternalServerError, gin.H{"error": "could not generate terraform plans summary"})
+ // Log error but continue as this is not a critical operation
}📝 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.
| err = CreateTerraformPlansSummary(d.GithubClientProvider, batch) | |
| if err != nil { | |
| log.Printf("could not generate terraform plans summary: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "could not generate terraform plans summary"}) | |
| } | |
| err = CreateTerraformPlansSummary(d.GithubClientProvider, batch) | |
| if err != nil { | |
| log.Printf("could not generate terraform plans summary: %v", err) | |
| // Log error but continue as this is not a critical operation | |
| } | |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/migrations/20241229112312.sql (1)
1-2: LGTM! Consider adding an index for ai_summary_comment_id.The schema changes look good. Both columns are appropriately nullable for backward compatibility.
Consider adding an index on
ai_summary_comment_idif you plan to query/join based on this column frequently, as it's likely to be used for lookups when processing GitHub comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
backend/migrations/atlas.sumis excluded by!**/*.sumcli/go.sumis excluded by!**/*.sumee/backend/go.sumis excluded by!**/*.sumee/cli/go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumlibs/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
backend/controllers/github.go(3 hunks)backend/migrations/20241229112312.sql(1 hunks)cli/go.mod(2 hunks)ee/backend/go.mod(8 hunks)ee/cli/go.mod(1 hunks)libs/go.mod(0 hunks)
💤 Files with no reviewable changes (1)
- libs/go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/controllers/github.go
- ee/backend/go.mod
🔇 Additional comments (2)
cli/go.mod (1)
Line range hint 58-84: LGTM! AWS SDK and GitLab package updates look consistent.
The dependency updates are well-coordinated:
- AWS SDK packages are updated to compatible versions
- Addition of go-gitlab package suggests GitLab integration capability
Run the following script to verify version compatibility:
✅ Verification successful
AWS SDK dependencies are properly configured and secure
The verification shows:
- No security vulnerabilities found in the AWS SDK packages
- AWS SDK dependencies are consistent across all packages (v1.32.4 base version)
- Only
configanddynamodbpackages are direct dependencies, others are indirect - AWS SDK is actively used across multiple components (storage, locking, scheduler) confirming these dependencies are necessary
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify AWS SDK package versions are compatible and check for any security advisories
# Check for any security advisories for AWS SDK
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/aws/aws-sdk-go-v2") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 360
Script:
#!/bin/bash
# Check if all AWS SDK packages are using consistent versions
rg "github.com/aws/aws-sdk-go-v2" cli/go.mod -A1 | grep -E "v[0-9]+\.[0-9]+\.[0-9]+" | sort
# Check if there are any direct (non-indirect) AWS SDK dependencies
rg "github.com/aws/aws-sdk-go-v2.*// indirect" -v cli/go.mod
# Check for any AWS SDK usage in the code
rg "aws-sdk-go-v2" -l
Length of output: 14926
ee/cli/go.mod (1)
61-84: LGTM! Version updates are synchronized with cli/go.mod.
The dependency updates maintain consistency with the main cli/go.mod:
- AWS SDK package versions match exactly
- All related AWS packages are updated together
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/controllers/projects.go (3)
680-691: Optimize Terraform outputs collectionThe current implementation uses string concatenation in a loop, which is inefficient. Consider using strings.Builder for better performance and add clear separators between project outputs:
-var terraformOutputs = "" +var builder strings.Builder for _, job := range jobs { var jobSpec orchestrator_scheduler.JobJson err := json.Unmarshal(job.SerializedJobSpec, &jobSpec) if err != nil { log.Printf("could not summarise plans due to unmarshalling error: %v", err) return fmt.Errorf("could not summarise plans due to unmarshalling error: %v", err) } projectName := jobSpec.ProjectName - terraformOutputs += fmt.Sprintf("terraform output for %v \n\n", projectName) + job.TerraformOutput + fmt.Fprintf(&builder, "=== Terraform output for %v ===\n%v\n\n", projectName, job.TerraformOutput) } +terraformOutputs := builder.String()
666-666: Simplify boolean comparisonThe comparison to bool constant can be simplified.
-if batch.Status == orchestrator_scheduler.BatchJobSucceeded && config.Reporting.AiSummary == true { +if batch.Status == orchestrator_scheduler.BatchJobSucceeded && config.Reporting.AiSummary {🧰 Tools
🪛 golangci-lint (1.62.2)
666-666: S1002: should omit comparison to bool constant, can be simplified to
config.Reporting.AiSummary(gosimple)
692-704: LGTM with minor suggestionsThe AI summary generation and comment update logic looks good. However, consider adding error handling for the EditComment call:
-prService.EditComment(batch.PrNumber, batch.AiSummaryCommentId, summary) +if err := prService.EditComment(batch.PrNumber, batch.AiSummaryCommentId, summary); err != nil { + log.Printf("could not edit summary comment: %v", err) + return fmt.Errorf("could not edit summary comment: %v", err) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/controllers/projects.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
backend/controllers/projects.go
666-666: S1002: should omit comparison to bool constant, can be simplified to config.Reporting.AiSummary
(gosimple)
🔇 Additional comments (3)
backend/controllers/projects.go (3)
20-20: LGTM!
The addition of the "os" package import is necessary for accessing environment variables and is properly placed in alphabetical order.
485-489: 🛠️ Refactor suggestion
Improve error handling for CreateTerraformPlansSummary
The current implementation continues execution after logging the error and sending a 500 response. This could lead to multiple status codes being sent if subsequent operations fail. Consider handling the error more gracefully:
err = CreateTerraformOutputsSummary(d.GithubClientProvider, batch)
if err != nil {
log.Printf("could not generate terraform plans summary: %v", err)
- c.JSON(http.StatusInternalServerError, gin.H{"error": "could not generate terraform plans summary"})
+ // Log error but continue as this is not a critical operation
}Likely invalid or redundant comment.
673-679: 🛠️ Refactor suggestion
Enhance environment variable validation
The code validates the summary endpoint but not the API token. Consider validating both environment variables consistently.
summaryEndpoint := os.Getenv("DIGGER_AI_SUMMARY_ENDPOINT")
+apiToken := os.Getenv("DIGGER_AI_SUMMARY_API_TOKEN")
if summaryEndpoint == "" {
log.Printf("could not generate AI summary, ai summary endpoint missing")
return fmt.Errorf("could not generate AI summary, ai summary endpoint missing")
}
-apiToken := os.Getenv("DIGGER_AI_SUMMARY_API_TOKEN")
+if apiToken == "" {
+ log.Printf("could not generate AI summary, api token missing")
+ return fmt.Errorf("could not generate AI summary, api token missing")
+}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/controllers/github.go (1)
Line range hint
667-667: Omit comparison to bool constant.Golang recommends comparing boolean variables directly, without
== true.- if batch.Status == orchestrator_scheduler.BatchJobSucceeded && config.Reporting.AiSummary == true { + if batch.Status == orchestrator_scheduler.BatchJobSucceeded && config.Reporting.AiSummary {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/controllers/github.go(4 hunks)backend/controllers/projects.go(3 hunks)libs/digger_config/digger_config.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
backend/controllers/projects.go
667-667: S1002: should omit comparison to bool constant, can be simplified to config.Reporting.AiSummary
(gosimple)
libs/digger_config/digger_config.go
38-38: ineffectual assignment to err
(ineffassign)
🔇 Additional comments (2)
backend/controllers/projects.go (1)
485-489: 🛠️ Refactor suggestion
Return or handle errors after creating Terraform outputs summary.
The code logs the error but continues execution. If the summary generation is critical, consider returning or otherwise handling the error to avoid silently failing this step.
Example fix:
err = CreateTerraformOutputsSummary(d.GithubClientProvider, batch)
if err != nil {
- log.Printf("could not generate terraform plans summary: %v", err)
-}
+ log.Printf("could not generate terraform plans summary: %v", err)
+ c.JSON(http.StatusInternalServerError, gin.H{"error": "Unable to generate terraform plans summary"})
+ return
}Likely invalid or redundant comment.
backend/controllers/github.go (1)
599-602: Looks good.
This block properly handles errors from loading Digger config and logs any failures. No action needed here.
| diggerYmlBytes, err := os.ReadFile(path.Join(dir, "digger.yml")) | ||
| if err != nil { | ||
| // if file doesn't exist look for digger.yaml instead | ||
| diggerYmlBytes, err = os.ReadFile(path.Join(dir, "digger.yaml")) |
There was a problem hiding this comment.
Handle error after reading secondary file.
The error assigned to err here is never checked. If reading digger.yaml also fails, the error will be silently ignored. Consider returning the error in this scenario to avoid unexpected behavior.
Apply this diff to ensure we don't ignore the second read error:
func ReadDiggerYmlFileContents(dir string) (string, error) {
var diggerYmlBytes []byte
diggerYmlBytes, err := os.ReadFile(path.Join(dir, "digger.yml"))
if err != nil {
// if file doesn't exist look for digger.yaml instead
- diggerYmlBytes, err = os.ReadFile(path.Join(dir, "digger.yaml"))
+ diggerYmlBytes, err = os.ReadFile(path.Join(dir, "digger.yaml"))
+ if err != nil {
+ return "", err
+ }
}
diggerYmlStr := string(diggerYmlBytes)
return diggerYmlStr, 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.
| diggerYmlBytes, err = os.ReadFile(path.Join(dir, "digger.yaml")) | |
| func ReadDiggerYmlFileContents(dir string) (string, error) { | |
| var diggerYmlBytes []byte | |
| diggerYmlBytes, err := os.ReadFile(path.Join(dir, "digger.yml")) | |
| if err != nil { | |
| // if file doesn't exist look for digger.yaml instead | |
| diggerYmlBytes, err = os.ReadFile(path.Join(dir, "digger.yaml")) | |
| if err != nil { | |
| return "", err | |
| } | |
| } | |
| diggerYmlStr := string(diggerYmlBytes) | |
| return diggerYmlStr, nil | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
38-38: ineffectual assignment to err
(ineffassign)
backend/controllers/github.go
Outdated
| var aiSummaryCommentId = "" | ||
| if config.Reporting.AiSummary { | ||
| aiSummaryComment, err := ghService.PublishComment(issueNumber, "AI Summary will be posted here after completion") | ||
| if err != nil { | ||
| log.Printf("could not post ai summary comment: %v", err) | ||
| commentReporterManager.UpdateComment(fmt.Sprintf(":x: could not post ai comment summary comment id: %v", err)) | ||
| } | ||
| aiSummaryCommentId = aiSummaryComment.Id | ||
| } | ||
|
|
There was a problem hiding this comment.
Check for nil after failing to publish the AI summary comment.
Similarly to the earlier comment, if PublishComment fails, referencing aiSummaryComment.Id may cause a panic.
Proposed fix:
var aiSummaryCommentId = ""
if config.Reporting.AiSummary {
aiSummaryComment, err := ghService.PublishComment(issueNumber, "AI Summary will be posted here after completion")
if err != nil {
log.Printf("could not post ai summary comment: %v", err)
commentReporterManager.UpdateComment(fmt.Sprintf(":x: could not post ai comment summary comment id: %v", err))
+ return
}
aiSummaryCommentId = aiSummaryComment.Id
}Committable suggestion skipped: line range outside the PR's diff.
backend/controllers/github.go
Outdated
|
|
||
| var aiSummaryCommentId = "" | ||
| if config.Reporting.AiSummary { | ||
| aiSummaryComment, err := ghService.PublishComment(prNumber, "AI Summary will be posted here after completion") | ||
| if err != nil { | ||
| log.Printf("could not post ai summary comment: %v", err) | ||
| commentReporterManager.UpdateComment(fmt.Sprintf(":x: could not post ai comment summary comment id: %v", err)) | ||
| } | ||
| aiSummaryCommentId = aiSummaryComment.Id | ||
| } | ||
|
|
There was a problem hiding this comment.
Check for nil after failing to publish the AI summary comment.
If PublishComment fails, we log the error but still proceed to use aiSummaryComment.Id, which may cause a nil pointer dereference. Consider returning early when an error occurs.
Proposed fix:
var aiSummaryCommentId = ""
if config.Reporting.AiSummary {
aiSummaryComment, err := ghService.PublishComment(prNumber, "AI Summary will be posted here after completion")
if err != nil {
log.Printf("could not post ai summary comment: %v", err)
commentReporterManager.UpdateComment(fmt.Sprintf(":x: could not post ai comment summary comment id: %v", err))
+ return
}
aiSummaryCommentId = aiSummaryComment.Id
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/controllers/projects.go (3)
485-489: Enhance error handling with more contextThe error handling could be more descriptive to aid in debugging.
err = CreateTerraformOutputsSummary(d.GithubClientProvider, batch) if err != nil { - log.Printf("could not generate terraform plans summary: %v", err) + log.Printf("could not generate terraform plans summary for batch %v: %v", batch.ID, err) }
685-695: Optimize string concatenation for better performanceUsing string concatenation in a loop can be inefficient. Consider using strings.Builder for better performance.
-var terraformOutputs = "## AI Summary for terraform runs\n\n" +var builder strings.Builder +builder.WriteString("## AI Summary for terraform runs\n\n") for _, job := range jobs { var jobSpec orchestrator_scheduler.JobJson err := json.Unmarshal(job.SerializedJobSpec, &jobSpec) if err != nil { log.Printf("could not summarise plans due to unmarshalling error: %v", err) return fmt.Errorf("could not summarise plans due to unmarshalling error: %v", err) } projectName := jobSpec.ProjectName - terraformOutputs += fmt.Sprintf("terraform output for %v \n\n", projectName) + job.TerraformOutput + fmt.Fprintf(&builder, "terraform output for %v \n\n%v", projectName, job.TerraformOutput) } +terraformOutputs := builder.String()
665-665: Simplify boolean comparisonThe boolean comparison can be simplified.
-if batch.Status == orchestrator_scheduler.BatchJobSucceeded && config.Reporting.AiSummary == true { +if batch.Status == orchestrator_scheduler.BatchJobSucceeded && config.Reporting.AiSummary {🧰 Tools
🪛 golangci-lint (1.62.2)
665-665: S1002: should omit comparison to bool constant, can be simplified to
config.Reporting.AiSummary(gosimple)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/controllers/projects.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
backend/controllers/projects.go
665-665: S1002: should omit comparison to bool constant, can be simplified to config.Reporting.AiSummary
(gosimple)
🔇 Additional comments (1)
backend/controllers/projects.go (1)
672-678: Enhance environment variable validation
The code validates the summary endpoint but not the API token. Consider validating both environment variables consistently.
summaryEndpoint := os.Getenv("DIGGER_AI_SUMMARY_ENDPOINT")
+apiToken := os.Getenv("DIGGER_AI_SUMMARY_API_TOKEN")
if summaryEndpoint == "" {
log.Printf("could not generate AI summary, ai summary endpoint missing")
return fmt.Errorf("could not generate AI summary, ai summary endpoint missing")
}
-apiToken := os.Getenv("DIGGER_AI_SUMMARY_API_TOKEN")
+if apiToken == "" {
+ log.Printf("could not generate AI summary, api token missing")
+ return fmt.Errorf("could not generate AI summary, api token missing")
+}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
backend/controllers/projects.go (4)
665-665: Simplify boolean comparisonThe boolean comparison can be simplified.
-if batch.Status == orchestrator_scheduler.BatchJobSucceeded && config.Reporting.AiSummary == true { +if batch.Status == orchestrator_scheduler.BatchJobSucceeded && config.Reporting.AiSummary {🧰 Tools
🪛 golangci-lint (1.62.2)
665-665: S1002: should omit comparison to bool constant, can be simplified to
config.Reporting.AiSummary(gosimple)
669-669: Standardize error message formattingThe error messages in EditComment/PublishComment calls have inconsistent formatting. Some have extra newlines, some don't. Consider standardizing the format.
-":x: could not generate AI summary \n\n could not communicate with github" -":x: could not generate AI summary \n\n could not communicate with github" -":x: could not generate AI summary \n\n comment ID is missing " -":x: could not generate AI summary \n\n error fetching jobs " -":x: could not generate AI summary \n\n error fetching job spec" -":x: could not generate AI summary \n\n error generating summary from plans" +":x: Could not generate AI summary\n\nReason: Could not communicate with GitHub" +":x: Could not generate AI summary\n\nReason: Could not communicate with GitHub" +":x: Could not generate AI summary\n\nReason: Comment ID is missing" +":x: Could not generate AI summary\n\nReason: Error fetching jobs" +":x: Could not generate AI summary\n\nReason: Error fetching job spec" +":x: Could not generate AI summary\n\nReason: Error generating summary from plans"Also applies to: 675-675, 682-682, 690-690, 700-700, 709-709
655-716: Consider breaking down the function for better maintainabilityThe function has high cyclomatic complexity with multiple nested conditions and error handling. Consider breaking it down into smaller, focused functions:
- validateConfiguration
- validateEnvironment
- collectTerraformOutputs
- generateAndPostSummary
Here's a sketch of how it could be refactored:
func validateConfiguration(batch *models.DiggerBatch) (*digger_config.DiggerConfig, error) { // Handle digger config loading and validation } func validateEnvironment() (string, string, error) { // Handle environment variables validation } func collectTerraformOutputs(batchID uint) (string, error) { // Handle collecting terraform outputs } func generateAndPostSummary(prService ci.PullRequestService, summary string, batch *models.DiggerBatch) error { // Handle posting the summary } func CreateTerraformOutputsSummary(gh utils.GithubClientProvider, batch *models.DiggerBatch) error { config, err := validateConfiguration(batch) if err != nil { return err } if batch.Status != orchestrator_scheduler.BatchJobSucceeded || !config.Reporting.AiSummary { return nil } summaryEndpoint, apiToken, err := validateEnvironment() if err != nil { return err } terraformOutputs, err := collectTerraformOutputs(batch.ID) if err != nil { return err } prService, err := GetPrServiceFromBatch(batch, gh) if err != nil { return err } summary, err := utils.GetAiSummaryFromTerraformPlans(terraformOutputs, summaryEndpoint, apiToken) if err != nil { return err } return generateAndPostSummary(prService, summary, batch) }🧰 Tools
🪛 golangci-lint (1.62.2)
665-665: S1002: should omit comparison to bool constant, can be simplified to
config.Reporting.AiSummary(gosimple)
694-705: Enhance terraform outputs formattingThe current formatting of terraform outputs could be improved for better readability.
-var terraformOutputs = "## AI Summary for terraform runs\n\n" +var terraformOutputs = "## AI Summary for Terraform Runs\n\n" for _, job := range jobs { // ... job spec unmarshaling ... projectName := jobSpec.ProjectName - terraformOutputs += fmt.Sprintf("terraform output for %v \n\n", projectName) + job.TerraformOutput + terraformOutputs += fmt.Sprintf("### Terraform Output: %s\n\n```hcl\n%s\n```\n\n", projectName, job.TerraformOutput) }libs/digger_config/digger_config.go (3)
33-45: Consider aligning with existing file handling patterns.While the implementation is functionally correct, consider these improvements for better consistency and maintainability:
- Use the existing
isFileExistshelper function to check file existence.- Align the implementation with
retrieveConfigFilefunction's pattern.- Make the error message more specific about which file failed.
func ReadDiggerYmlFileContents(dir string) (string, error) { - var diggerYmlBytes []byte - diggerYmlBytes, err := os.ReadFile(path.Join(dir, "digger.yml")) - if err != nil { - // if file doesn't exist look for digger.yaml instead - diggerYmlBytes, err = os.ReadFile(path.Join(dir, "digger.yaml")) - if err != nil { - return "", fmt.Errorf("could not read the file both digger.yml and digger.yaml are missing: %v", err) - } - } - diggerYmlStr := string(diggerYmlBytes) - return diggerYmlStr, nil + ymlPath := path.Join(dir, "digger.yml") + yamlPath := path.Join(dir, "digger.yaml") + + if isFileExists(ymlPath) { + diggerYmlBytes, err := os.ReadFile(ymlPath) + if err != nil { + return "", fmt.Errorf("failed to read digger.yml: %v", err) + } + return string(diggerYmlBytes), nil + } + + if isFileExists(yamlPath) { + diggerYmlBytes, err := os.ReadFile(yamlPath) + if err != nil { + return "", fmt.Errorf("failed to read digger.yaml: %v", err) + } + return string(diggerYmlBytes), nil + } + + return "", fmt.Errorf("neither digger.yml nor digger.yaml exists in directory: %s", dir) }
33-45: Consider consolidating configuration file handling logic.The new
ReadDiggerYmlFileContentsfunction partially duplicates logic fromretrieveConfigFile. Consider refactoring to share common functionality:
- Extract the file existence checking logic into a shared helper.
- Use
retrieveConfigFileto get the correct filename, then read its contents.Example approach:
// Helper function to consolidate file handling logic func getDiggerConfigPath(dir string) (string, error) { configPath, err := retrieveConfigFile(dir) if err != nil { return "", err } if configPath == "" { return "", fmt.Errorf("neither digger.yml nor digger.yaml exists in directory: %s", dir) } return configPath, nil } func ReadDiggerYmlFileContents(dir string) (string, error) { configPath, err := getDiggerConfigPath(dir) if err != nil { return "", err } data, err := os.ReadFile(configPath) if err != nil { return "", fmt.Errorf("failed to read %s: %v", configPath, err) } return string(data), nil }
33-45: Enhance error handling with error wrapping.While the error handling is functional, consider using error wrapping to preserve the error chain for better debugging:
func ReadDiggerYmlFileContents(dir string) (string, error) { var diggerYmlBytes []byte diggerYmlBytes, err := os.ReadFile(path.Join(dir, "digger.yml")) if err != nil { // if file doesn't exist look for digger.yaml instead diggerYmlBytes, err = os.ReadFile(path.Join(dir, "digger.yaml")) if err != nil { - return "", fmt.Errorf("could not read the file both digger.yml and digger.yaml are missing: %v", err) + return "", fmt.Errorf("could not read configuration files: %w", err) } } diggerYmlStr := string(diggerYmlBytes) return diggerYmlStr, nil }This change:
- Uses
%winstead of%vto wrap the original error- Preserves the full error chain for better debugging
- Allows upstream code to use
errors.Isanderrors.Asfor error handling
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/controllers/projects.go(3 hunks)libs/digger_config/digger_config.go(1 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.
🪛 golangci-lint (1.62.2)
backend/controllers/projects.go
665-665: S1002: should omit comparison to bool constant, can be simplified to config.Reporting.AiSummary
(gosimple)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/controllers/github.go (1)
513-524: 🛠️ Refactor suggestionApply the same refactoring to handlePullRequestEvent.
The same issues exist here as in
handleIssueCommentEvent. Use the extracted helper function here as well.
🧹 Nitpick comments (2)
backend/controllers/github.go (2)
785-788: Enhance error handling for terraform generation.The error handling could be more informative and follow Go's error handling best practices.
if err != nil { - log.Printf("terraform generation failed: %v", err) - return err + log.Printf("Terraform generation failed for project: %v, error: %v", projectName, err) + return fmt.Errorf("terraform generation failed: %w", err) }
599-603: Enhance error handling for configuration file reading.The error handling could be more informative by including the directory context and wrapping the error.
diggerYmlStr, err = dg_configuration.ReadDiggerYmlFileContents(dir) if err != nil { - log.Printf("could not load digger config: %v", err) - return err + log.Printf("could not load digger config from directory %s: %v", dir, err) + return fmt.Errorf("failed to read digger.yml from %s: %w", dir, err) }
| var aiSummaryCommentId = "" | ||
| if config.Reporting.AiSummary { | ||
| aiSummaryComment, err := ghService.PublishComment(issueNumber, "AI Summary will be posted here after completion") | ||
| if err != nil { | ||
| log.Printf("could not post ai summary comment: %v", err) | ||
| commentReporterManager.UpdateComment(fmt.Sprintf(":x: could not post ai comment summary comment id: %v", err)) | ||
| return fmt.Errorf("could not post ai summary comment: %v", err) | ||
| } | ||
| aiSummaryCommentId = aiSummaryComment.Id | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated AI summary initialization logic.
The AI summary initialization logic is duplicated between handleIssueCommentEvent and handlePullRequestEvent. Consider extracting this into a shared helper function.
+func initializeAISummary(config *dg_configuration.DiggerConfig, ghService *dg_github.GithubService, issueNumber int) (string, error) {
+ if !config.Reporting.AiSummary {
+ return "", nil
+ }
+ aiSummaryComment, err := ghService.PublishComment(issueNumber, "AI Summary will be posted here after completion")
+ if err != nil {
+ return "", fmt.Errorf("could not post ai summary comment: %w", err)
+ }
+ return aiSummaryComment.Id, nil
+}Then use it in both handlers:
-var aiSummaryCommentId = ""
-if config.Reporting.AiSummary {
- aiSummaryComment, err := ghService.PublishComment(issueNumber, "AI Summary will be posted here after completion")
- if err != nil {
- log.Printf("could not post ai summary comment: %v", err)
- commentReporterManager.UpdateComment(fmt.Sprintf(":x: could not post ai comment summary comment id: %v", err))
- return fmt.Errorf("could not post ai summary comment: %v", err)
- }
- aiSummaryCommentId = aiSummaryComment.Id
-}
+aiSummaryCommentId, err := initializeAISummary(config, ghService, issueNumber)
+if err != nil {
+ log.Printf("could not initialize ai summary: %v", err)
+ commentReporterManager.UpdateComment(fmt.Sprintf(":x: could not initialize ai summary: %v", err))
+ return fmt.Errorf("could not initialize ai summary: %w", err)
+}Committable suggestion skipped: line range outside the PR's diff.
* add flag to ignore all external directories per project (#1851) * add flag to ignore all external directories per project * revert includeparentblocks flag (#1852) * improve efficiency in terragrunt generation (#1854) * improve efficiency in terragrunt generation * Update action.yml (#1856) * handle crashes in goroutine events (#1857) * fix/recover from webhook goroutines (#1858) * handle crashes in goroutine events * include stacktrace in errors * wip generation of terraform code from application code (#1855) * terraform code generation demo --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: incorrect comment in backend/migrations (#1860) * Update setup-opentofu to fix issues with 1.6.x downloads (#1861) * restructure docs to have no columns (#1862) * Create curl_bootstrap.sh * refactor codegen parts (#1863) * refactor codegen parts * publish ai summaries (#1864) * publish ai summaries * add a header for summary comment (#1865) * fix heading (#1866) --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aldo <82811+aldoborrero@users.noreply.github.com> Co-authored-by: Hugo Samayoa <htplbc@gmail.com>
Summary by CodeRabbit
Release Notes
New Features
digger.ymlordigger.yaml).Configuration Updates
Improvements