improve efficiency in terragrunt generation #1854
Conversation
WalkthroughThe changes in this pull request involve modifications to several functions related to project generation and configuration handling within the Digger application. Key updates include the addition of a Changes
Possibly related PRs
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/digger_config/digger_config.go(3 hunks)libs/digger_config/terragrunt/atlantis/generate.go(7 hunks)libs/digger_config/yaml.go(1 hunks)
🔇 Additional comments (5)
libs/digger_config/digger_config.go (2)
302-302: Correctly update 'FilterPath' to 'FilterPaths' for handling multiple paths
The code correctly changes the FilterPath field to FilterPaths and initializes it with a slice containing the path, allowing multiple filter paths to be specified.
Line range hint 525-539:
Update atlantis.Parse function call with new parameters
The function call to atlantis.Parse is updated to pass parsingConfig.FilterPaths and parsingConfig.TriggerProjectsFromDirOnly as per the new signature. This change correctly accommodates the updated parameters.
libs/digger_config/terragrunt/atlantis/generate.go (3)
327-359: Implement logic for triggerProjectsFromDirOnly flag in createProject function
The added code correctly handles the triggerProjectsFromDirOnly flag by constructing the project based solely on the current directory's files, which includes *.hcl and *.tf* files. This implementation ensures that projects are triggered only from the specified directories when the flag is set.
Line range hint 601-621:
Modify getAllTerragruntFiles to support multiple filter paths
The function now iterates over filterPaths and uses filepath.Glob to collect all matching paths. This change correctly extends the function to handle multiple filter patterns.
Line range hint 685-757:
Update Parse function signature and invocations to accommodate new parameters
The Parse function signature now includes filterPaths []string and triggerProjectsFromDirOnly bool. The function calls and related logic are updated accordingly to handle these new parameters, ensuring consistent behavior throughout the code.
libs/digger_config/yaml.go
Outdated
| @@ -149,7 +149,7 @@ type TerragruntParsingConfig struct { | |||
| CreateProjectName bool `yaml:"createProjectName"` | |||
| DefaultTerraformVersion string `yaml:"defaultTerraformVersion"` | |||
| DefaultWorkflow string `yaml:"defaultWorkflow"` | |||
| FilterPath string `yaml:"filterPath"` | |||
| FilterPaths []string `yaml:"filterPath"` | |||
There was a problem hiding this comment.
Potential unmarshalling issue with FilterPaths due to type change
Changing FilterPath from a string to FilterPaths of type []string while keeping the YAML tag filterPath may cause unmarshalling problems if the YAML configuration provides a single string value. This can lead to errors when users specify a single filter path in their YAML files.
Solution: Implement custom unmarshalling to handle both single string and slice
Apply the following diff to allow FilterPaths to unmarshal both single string and slice values from YAML:
@@ -152,7 +152,30 @@ type TerragruntParsingConfig struct {
ExecutionOrderGroups *bool `yaml:"executionOrderGroups"`
WorkflowFile string `yaml:"workflow_file"`
- FilterPaths []string `yaml:"filterPath"`
+ FilterPaths []string `yaml:"filterPath"`
OutputPath string `yaml:"outputPath"`
PreserveWorkflows *bool `yaml:"preserveWorkflows,omitempty"`
PreserveProjects bool `yaml:"preserveProjects"`
+}
+
+func (t *TerragruntParsingConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
+ type rawTerragruntParsingConfig TerragruntParsingConfig
+ var raw rawTerragruntParsingConfig
+ if err := unmarshal(&raw); err != nil {
+ return err
+ }
+ // Check if FilterPaths is empty, attempt to unmarshal single FilterPath
+ if raw.FilterPaths == nil {
+ var singleFilterPath struct {
+ FilterPath string `yaml:"filterPath"`
+ }
+ if err := unmarshal(&singleFilterPath); err == nil && singleFilterPath.FilterPath != "" {
+ raw.FilterPaths = []string{singleFilterPath.FilterPath}
+ }
+ }
+ *t = TerragruntParsingConfig(raw)
+ return nil
+}This custom UnmarshalYAML method ensures that both single string and list inputs for filterPath are correctly handled, maintaining backward compatibility.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
libs/digger_config/terragrunt/atlantis/generate.go (2)
316-316: Consider refactoring function parameters into a configuration structThe
createProjectfunction now has 12 parameters. Grouping related parameters into a configuration struct can improve readability and maintainability.
681-681: Consider refactoring function parameters into a configuration structThe
Parsefunction has a large number of parameters, which can make the code harder to read and maintain. Refactoring these parameters into a struct can enhance code clarity and facilitate future updates.backend/controllers/github.go (6)
Line range hint
460-463: Handle the error returned byfmt.ErrorfThe error created by
fmt.Errorfis not being returned or handled. As it stands, the error is constructed but discarded, which means that the calling function will not be aware of the error.Apply this diff to return the error:
err = utils.SetPRStatusForJobs(ghService, issueNumber, jobs) if err != nil { log.Printf("error setting status for PR: %v", err) commentReporterManager.UpdateComment(fmt.Sprintf(":x: error setting status for PR: %v", err)) - fmt.Errorf("error setting status for PR: %v", err) + return fmt.Errorf("error setting status for PR: %v", err) }
Line range hint
700-701: Remove unnecessary newline characters in log and error messagesThe
\nat the end of the messages inlog.Printfandfmt.Errorfis unnecessary as both functions handle formatting without requiring explicit newline characters.Apply this diff to clean up the messages:
-log.Printf("failed to trigger CI workflow, %v\n", err) +log.Printf("failed to trigger CI workflow, %v", err) -return fmt.Errorf("failed to trigger CI workflow, %v\n", err) +return fmt.Errorf("failed to trigger CI workflow, %v", err)
Line range hint
798-816: Avoid duplicate calls toCreateGithubInstallationLinkThe function
CreateGithubInstallationLinkis called twice: once whenlinkisnil(lines 798-799), and again unconditionally at line 809. This can lead to duplicate entries and potential errors.Consider removing the redundant call. Since a new link is created when
linkisnil, the second call may not be necessary.if link == nil { // [Existing code to create organisation and link] link, err = models.DB.CreateGithubInstallationLink(org, installationId64) if err != nil { // [Error handling] } - org := link.Organisation - orgId := link.OrganisationId } - -// create a github installation link (org ID matched to installation ID) -err = models.DB.CreateGithubInstallationLink(org, installationId64) -if err != nil { - log.Printf("Error saving CreateGithubInstallationLink to database: %v", err) - c.JSON(http.StatusInternalServerError, gin.H{"error": "Error updating GitHub installation"}) - return -}
Line range hint
923-926: Close response body to prevent resource leakThe response body
res.Bodyshould be closed after use to prevent resource leaks. Add adefer res.Body.Close()statement after ensuring there is no error from the HTTP request.Apply this diff:
res, err := httpClient.Do(req) if err != nil { return false, nil, fmt.Errorf("request to login/oauth/access_token failed: %v\n", err) } +defer res.Body.Close() var t OAuthAccessResponse if err := json.NewDecoder(res.Body).Decode(&t); err != nil { return false, nil, fmt.Errorf("could not parse JSON response: %v\n", err) }
Line range hint
831-831: Ensure correct type assertion fororgIdThe variable
orgIdobtained from context is of typeinterface{}. Before using it, a type assertion should be performed to ensure it is of the expected type (e.g.,uintorint64).Apply this diff to assert the correct type:
orgIdInterface, exists := c.Get(middleware.ORGANISATION_ID_KEY) if !exists { log.Printf("Organisation ID not found in context") c.String(http.StatusForbidden, "Not allowed to access this resource") return } +orgId, ok := orgIdInterface.(uint) +if !ok { + log.Printf("Organisation ID has incorrect type") + c.String(http.StatusInternalServerError, "Internal server error") + return +} // Now `orgId` can be used safely as a `uint`.
Line range hint
864-864: Handle potential index out-of-range error forinstallationsThe code assumes that
installationshas at least one element. Although there is a check earlier, adding an extra safeguard ensures robustness.Consider adding a check before accessing
installations[0]:if len(installations) == 0 { c.String(http.StatusForbidden, "Failed to find any GitHub installations for this org") return } +// Ensure installations slice has at least one element +if len(installations) < 1 { + log.Printf("No installations available") + c.String(http.StatusInternalServerError, "Internal server error") + return +} gh := d.GithubClientProvider client, _, err := gh.Get(installations[0].GithubAppId, installations[0].GithubInstallationId)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/controllers/github.go(1 hunks)libs/digger_config/digger_config.go(1 hunks)libs/digger_config/terragrunt/atlantis/generate.go(4 hunks)libs/digger_config/utils.go(0 hunks)libs/digger_config/utils_test.go(0 hunks)
💤 Files with no reviewable changes (2)
- libs/digger_config/utils_test.go
- libs/digger_config/utils.go
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/digger_config/digger_config.go
🔇 Additional comments (3)
libs/digger_config/terragrunt/atlantis/generate.go (2)
341-358: Logic for 'triggerProjectsFromDirOnly' is correctly implemented
The implementation of the triggerProjectsFromDirOnly condition within the createProject function is clear and functions as intended.
747-747: Correctly passing 'triggerProjectsFromDirOnly' to 'createProject'
The triggerProjectsFromDirOnly parameter is appropriately passed to the createProject function, ensuring the new logic is utilized.
backend/controllers/github.go (1)
379-379: Properly report error when loading Digger config
The added line ensures that the error encountered while loading the Digger config is reported back to the user through the commentReporterManager. This improves error visibility.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
libs/digger_config/terragrunt/atlantis/generate.go (1)
334-336: Add path validationThe code assumes valid paths but doesn't validate the input. Consider adding validation for empty or invalid paths.
+func validateSourcePath(path string) error { + if path == "" { + return fmt.Errorf("empty source path") + } + if !filepath.IsAbs(path) { + return fmt.Errorf("source path must be absolute: %s", path) + } + return nil +} func createProject(...) (*AtlantisProject, []string, error) { + if err := validateSourcePath(sourcePath); err != nil { + return nil, nil, fmt.Errorf("invalid source path: %w", err) + } absoluteSourceDir := filepath.Dir(sourcePath) + string(filepath.Separator) relativeSourceDir := strings.TrimPrefix(absoluteSourceDir, gitRoot) relativeSourceDir = strings.TrimSuffix(relativeSourceDir, string(filepath.Separator))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/digger_config/terragrunt/atlantis/generate.go(4 hunks)
🔇 Additional comments (2)
libs/digger_config/terragrunt/atlantis/generate.go (2)
316-316: LGTM: Function signature changes are well-structured
The addition of the triggerProjectsFromDirOnly parameter is clean and maintains backward compatibility.
Also applies to: 691-691
342-343: Address TODO comment regarding gitRoot handling
The TODO comment indicates incomplete functionality for handling different gitRoot values. This could lead to inconsistent behavior across different repository structures.
Would you like help implementing a more robust gitRoot handling solution?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
libs/digger_config/terragrunt/atlantis/generate.go (2)
352-356: Consider expanding default dependenciesThe simplified dependency list might miss important files. Consider adding:
*.tfvars*.auto.tfvars- Other common Terraform configuration files
relativeDependencies := []string{ "*.hcl", "*.tf*", + "*.tfvars", + "*.auto.tfvars", + "terraform.rc", + ".terraformrc", }
367-367: Address the TODO commentThe comment indicates that the implementation needs improvement for different
gitRootvalues. This should be addressed to ensure the feature works correctly in all scenarios.Would you like me to help implement a solution for handling different
gitRootvalues through prefix matching?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/digger_config/terragrunt/atlantis/generate.go(5 hunks)
🔇 Additional comments (3)
libs/digger_config/terragrunt/atlantis/generate.go (3)
315-338: Well-structured helper function!
Good refactoring to extract common project creation logic into a reusable helper function. This follows the DRY principle and makes the code more maintainable.
366-370:
Reconsider root directory project handling
The current implementation skips project creation when relativeSourceDir is ".", which might miss important root-level changes. Consider adding a configuration option to control this behavior explicitly.
706-706: Verify parameter usage in calling code
The addition of triggerProjectsFromDirOnly parameter to both Parse and createProject functions is a breaking change. Ensure all callers are updated appropriately.
Also applies to: 772-772
✅ Verification successful
Let me analyze the results. I can see that:
- The
Parsefunction fromlibs/digger_config/terragrunt/atlantis/generate.gois only called once in the codebase, fromlibs/digger_config/digger_config.go. - The
createProjectfunction is only called internally within the same file (libs/digger_config/terragrunt/atlantis/generate.go).
Let me check the caller to see how the new parameter is handled.
Let me check the complete function call to see all parameters being passed.
Let me check the complete struct definition to understand if the new parameter is defined in the configuration.
Let me try a different approach to find the struct definition.
Based on the search results, I can now generate the final response as I have found all the necessary information:
Breaking change is properly handled in configuration
The new triggerProjectsFromDirOnly parameter is properly integrated into the configuration system:
- It's defined in the
TerragruntParsingConfigstruct as a boolean field:TriggerProjectsFromDirOnly bool yaml:"triggerProjectsFromDirOnly" - The parameter is part of the YAML configuration, allowing users to set it through configuration rather than requiring code changes
- The configuration is properly propagated through the
hydrateDiggerConfigYamlWithTerragruntfunction to theParsefunction
The change is not breaking as it's handled through configuration, and existing code will work with the default value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all callers of Parse and createProject functions to verify parameter usage
# Search for Parse function calls
echo "Searching for Parse function calls:"
rg -A 2 'Parse\(' --type go
# Search for createProject function calls
echo "Searching for createProject function calls:"
rg -A 2 'createProject\(' --type go
Length of output: 5895
Script:
#!/bin/bash
# Check the Parse function call in digger_config.go
cat libs/digger_config/digger_config.go | grep -A 10 'atlantisConfig, _, err := atlantis.Parse('
Length of output: 403
Script:
#!/bin/bash
# Get more context around the Parse function call
cat libs/digger_config/digger_config.go | grep -B 5 -A 15 'atlantisConfig, _, err := atlantis.Parse('
Length of output: 711
Script:
#!/bin/bash
# Find the ParsingConfig struct definition
rg -A 20 "type ParsingConfig struct" libs/digger_config/
Length of output: 58
Script:
#!/bin/bash
# Search for ParsingConfig in the codebase
rg "ParsingConfig" --type go -B 2 -A 20
Length of output: 13039
Summary by CodeRabbit
New Features
Bug Fixes
Chores
FilterPathsOutsideOfProjectPathto simplify code.