-
Notifications
You must be signed in to change notification settings - Fork 562
iterative caching for terragrunt configs #2051
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
51d8172
f087dca
2aa43d1
a3b4695
c53fb96
111a069
1876af4
e2b798b
00a08ff
09903bc
fa2ac3e
4d21b68
8a1b2bd
cf0f6d3
414b330
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,10 +1,15 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
package controllers | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"bytes" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"encoding/json" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"fmt" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"github.com/diggerhq/digger/libs/digger_config/terragrunt/tac" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"github.com/diggerhq/digger/libs/git_utils" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"io" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"log/slog" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"net/http" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"net/url" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"os" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"path" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"strings" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -15,13 +20,14 @@ import ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"github.com/gin-gonic/gin" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
type UpdateCacheRequest struct { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
RepoFullName string `json:"repo_full_name"` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Branch string `json:"branch"` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
OrgId uint `json:"org_id"` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
InstallationId int64 `json:"installation_id"` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
func (d DiggerController) UpdateRepoCache(c *gin.Context) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
type UpdateCacheRequest struct { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
RepoFullName string `json:"repo_full_name"` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Branch string `json:"branch"` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
OrgId uint `json:"org_id"` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
InstallationId int64 `json:"installation_id"` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
var request UpdateCacheRequest | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
err := c.BindJSON(&request) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -65,13 +71,14 @@ func (d DiggerController) UpdateRepoCache(c *gin.Context) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
var diggerYmlStr string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
var config *dg_configuration.DiggerConfig | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
var newAtlantisConfig *tac.AtlantisConfig | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// update the cache here, do it async for immediate response | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
go func() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
err = git_utils.CloneGitRepoAndDoAction(cloneUrl, branch, "", *token, "", func(dir string) error { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
diggerYmlBytes, err := os.ReadFile(path.Join(dir, "digger.yml")) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
diggerYmlStr = string(diggerYmlBytes) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
config, _, _, err = dg_configuration.LoadDiggerConfig(dir, true, nil) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
config, _, _, newAtlantisConfig, err = dg_configuration.LoadDiggerConfig(dir, true, nil, nil) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
slog.Error("Error loading digger config", "error", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -83,7 +90,7 @@ func (d DiggerController) UpdateRepoCache(c *gin.Context) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
slog.Error("Could not load digger config", "error", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
_, err = models.DB.UpsertRepoCache(orgId, repoFullName, diggerYmlStr, *config) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
_, err = models.DB.UpsertRepoCache(orgId, repoFullName, diggerYmlStr, *config, newAtlantisConfig) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the The fix adds a nil check before calling
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a potential nil pointer dereference bug in the If the
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: dereferencing *config without nil check could cause panic if LoadDiggerConfig returns nil config with non-nil error There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In I've been trying to find the implementation of If Since I can't see the implementation of
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is no dereferencing happening in that function There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In backend/controllers/cache.go, line 87 passes However, after examining the code, I cannot confirm this is a bug because:
Without seeing the implementation of
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
slog.Error("Could not update repo cache", "error", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -93,3 +100,72 @@ func (d DiggerController) UpdateRepoCache(c *gin.Context) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
c.String(http.StatusOK, "successfully submitted cache for processing, check backend logs for progress") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
func sendProcessCacheRequest(repoFullName string, branch string, installationId int64) error { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
diggerHostname := os.Getenv("HOSTNAME") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
webhookSecret := os.Getenv("DIGGER_INTERNAL_SECRET") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+105
to
+106
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The If these environment variables are not set:
This could lead to silent failures or unexpected behavior in the cache update process. The fix adds validation for both environment variables and returns appropriate error messages if they're not set.
Suggested change
Comment on lines
+105
to
+106
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
Comment on lines
+105
to
+106
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The The fix adds validation to check if the
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
installationLink, err := models.DB.GetGithubInstallationLinkForInstallationId(installationId) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
slog.Error("Error getting installation link", "installationId", installationId, "error", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
orgId := installationLink.OrganisationId | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
payload := UpdateCacheRequest{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
RepoFullName: repoFullName, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Branch: branch, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
InstallationId: installationId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
OrgId: orgId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
cacheRefreshUrl, err := url.JoinPath(diggerHostname, "_internal/update_repo_cache") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
slog.Error("Error joining URL paths", "error", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+105
to
+127
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The While The fix adds validation to check if the
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
jsonPayload, err := json.Marshal(payload) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
slog.Error("Process Cache: error marshaling JSON", "error", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
req, err := http.NewRequest("POST", cacheRefreshUrl, bytes.NewBuffer(jsonPayload)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
slog.Error("Process Cache: Error creating request", "error", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
req.Header.Set("Content-Type", "application/json") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
req.Header.Set("Authorization", fmt.Sprintf("Bearer %v", webhookSecret)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
client := &http.Client{} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
resp, err := client.Do(req) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+144
to
+145
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The HTTP client in Adding a reasonable timeout (30 seconds in this case) ensures that requests don't hang indefinitely. This is especially important for internal service communication where a non-responsive endpoint shouldn't block the entire system. Note that this change requires adding
Suggested change
Comment on lines
+144
to
+145
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The HTTP client in Adding a reasonable timeout (30 seconds in this case) ensures that requests don't hang indefinitely and resources are properly released if the server doesn't respond in a timely manner.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
fmt.Println("Error sending request:", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+144
to
+149
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the third instance of inconsistent error logging in the The fix standardizes this error logging to use
Suggested change
Comment on lines
+145
to
+149
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
defer resp.Body.Close() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+145
to
+150
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the The fix adds a nil check in the deferred function to ensure we only attempt to close the response body if both
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
statusCode := resp.StatusCode | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if statusCode != 200 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// Read response body to get error details | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
responseBody, err := io.ReadAll(resp.Body) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
slog.Error("Failed to read error response body", "error", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
slog.Error("got unexpected cache status", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"statusCode", statusCode, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"repoFullName", repoFullName, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"orgId", orgId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"branch", branch, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"installationId", installationId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"responseBody", string(responseBody)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+154
to
+166
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the If the error reading the response body occurs, we should return early with an appropriate error message rather than continuing with potentially invalid data. This ensures that the error handling is more robust and prevents using potentially corrupted or empty data in subsequent operations.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return fmt.Errorf("cache update failed with status code %d: %s", statusCode, string(responseBody)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+154
to
+168
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the Since The fix adds an early return with a more accurate error message when the response body cannot be read, ensuring that the error propagated up the call stack correctly reflects what actually happened.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ import ( | |
"encoding/json" | ||
"errors" | ||
"fmt" | ||
"github.com/diggerhq/digger/libs/digger_config/terragrunt/tac" | ||
"github.com/diggerhq/digger/libs/git_utils" | ||
"log/slog" | ||
"math/rand" | ||
|
@@ -416,7 +417,6 @@ func handlePushEvent(gh utils.GithubClientProvider, payload *github.PushEvent, a | |
loadProjectsOnPush := os.Getenv("DIGGER_LOAD_PROJECTS_ON_PUSH") | ||
|
||
if loadProjectsOnPush == "true" { | ||
|
||
if strings.HasSuffix(ref, defaultBranch) { | ||
slog.Debug("Loading projects from GitHub repo (push event)", "loadProjectsOnPush", loadProjectsOnPush, "ref", ref, "defaultBranch", defaultBranch) | ||
err := services.LoadProjectsFromGithubRepo(gh, strconv.FormatInt(installationId, 10), repoFullName, repoOwner, repoName, cloneURL, defaultBranch) | ||
|
@@ -428,6 +428,15 @@ func handlePushEvent(gh utils.GithubClientProvider, payload *github.PushEvent, a | |
slog.Debug("Skipping loading projects from GitHub repo", "loadProjectsOnPush", loadProjectsOnPush) | ||
} | ||
|
||
repoCacheEnabled := os.Getenv("DIGGER_CONFIG_REPO_CACHE_ENABLED") | ||
if repoCacheEnabled == "1" && strings.HasSuffix(ref, defaultBranch) { | ||
go func() { | ||
if err := sendProcessCacheRequest(repoFullName, defaultBranch, installationId); err != nil { | ||
slog.Error("Failed to process cache request", "error", err, "repoFullName", repoFullName) | ||
} | ||
}() | ||
} | ||
|
||
return nil | ||
} | ||
|
||
|
@@ -914,7 +923,7 @@ func handlePullRequestEvent(gh utils.GithubClientProvider, payload *github.PullR | |
return nil | ||
} | ||
|
||
func GetDiggerConfigForBranch(gh utils.GithubClientProvider, installationId int64, repoFullName string, repoOwner string, repoName string, cloneUrl string, branch string, changedFiles []string) (string, *dg_github.GithubService, *dg_configuration.DiggerConfig, graph.Graph[string, dg_configuration.Project], error) { | ||
func GetDiggerConfigForBranch(gh utils.GithubClientProvider, installationId int64, repoFullName string, repoOwner string, repoName string, cloneUrl string, branch string, changedFiles []string, taConfig *tac.AtlantisConfig) (string, *dg_github.GithubService, *dg_configuration.DiggerConfig, graph.Graph[string, dg_configuration.Project], error) { | ||
slog.Info("Getting Digger config for branch", | ||
slog.Group("repository", | ||
slog.String("fullName", repoFullName), | ||
|
@@ -954,7 +963,7 @@ func GetDiggerConfigForBranch(gh utils.GithubClientProvider, installationId int6 | |
|
||
slog.Debug("Successfully read digger.yml file", "configLength", len(diggerYmlStr)) | ||
|
||
config, _, dependencyGraph, err = dg_configuration.LoadDiggerConfig(dir, true, changedFiles) | ||
config, _, dependencyGraph, _, err = dg_configuration.LoadDiggerConfig(dir, true, changedFiles, taConfig) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: The LoadDiggerConfig call now passes taConfig as the 4th parameter, but the return values suggest it now returns 5 values instead of 4. Verify this matches the updated function signature. |
||
if err != nil { | ||
slog.Error("Error loading and parsing Digger config", | ||
"directory", dir, | ||
|
@@ -1046,6 +1055,7 @@ func getDiggerConfigForPR(gh utils.GithubClientProvider, orgId uint, prLabels [] | |
|
||
// check if items should be loaded from cache | ||
useCache := false | ||
var taConfig *tac.AtlantisConfig = nil | ||
if val, _ := os.LookupEnv("DIGGER_CONFIG_REPO_CACHE_ENABLED"); val == "1" && !slices.Contains(prLabels, "digger:no-cache") { | ||
useCache = true | ||
slog.Info("Attempting to load config from cache", | ||
|
@@ -1054,7 +1064,7 @@ func getDiggerConfigForPR(gh utils.GithubClientProvider, orgId uint, prLabels [] | |
"prNumber", prNumber, | ||
) | ||
|
||
diggerYmlStr, config, dependencyGraph, err := retrieveConfigFromCache(orgId, repoFullName) | ||
_, _, _, taConfigTemp, err := retrieveConfigFromCache(orgId, repoFullName) | ||
if err != nil { | ||
slog.Info("Could not load from cache, falling back to live loading", | ||
"orgId", orgId, | ||
|
@@ -1065,9 +1075,8 @@ func getDiggerConfigForPR(gh utils.GithubClientProvider, orgId uint, prLabels [] | |
slog.Info("Successfully loaded config from cache", | ||
"orgId", orgId, | ||
"repoFullName", repoFullName, | ||
"projectCount", len(config.Projects), | ||
) | ||
return diggerYmlStr, ghService, config, *dependencyGraph, &prBranch, &prCommitSha, changedFiles, nil | ||
taConfig = taConfigTemp | ||
} | ||
} | ||
|
||
|
@@ -1084,7 +1093,7 @@ func getDiggerConfigForPR(gh utils.GithubClientProvider, orgId uint, prLabels [] | |
"prNumber", prNumber, | ||
) | ||
|
||
diggerYmlStr, ghService, config, dependencyGraph, err := GetDiggerConfigForBranch(gh, installationId, repoFullName, repoOwner, repoName, cloneUrl, prBranch, changedFiles) | ||
diggerYmlStr, ghService, config, dependencyGraph, err := GetDiggerConfigForBranch(gh, installationId, repoFullName, repoOwner, repoName, cloneUrl, prBranch, changedFiles, taConfig) | ||
if err != nil { | ||
slog.Error("Error loading Digger config from repository", | ||
"prNumber", prNumber, | ||
|
@@ -1098,7 +1107,7 @@ func getDiggerConfigForPR(gh utils.GithubClientProvider, orgId uint, prLabels [] | |
return diggerYmlStr, ghService, config, dependencyGraph, &prBranch, &prCommitSha, changedFiles, nil | ||
} | ||
|
||
func retrieveConfigFromCache(orgId uint, repoFullName string) (string, *dg_configuration.DiggerConfig, *graph.Graph[string, dg_configuration.Project], error) { | ||
func retrieveConfigFromCache(orgId uint, repoFullName string) (string, *dg_configuration.DiggerConfig, *graph.Graph[string, dg_configuration.Project], *tac.AtlantisConfig, error) { | ||
slog.Debug("Retrieving config from cache", | ||
"orgId", orgId, | ||
"repoFullName", repoFullName, | ||
|
@@ -1111,7 +1120,7 @@ func retrieveConfigFromCache(orgId uint, repoFullName string) (string, *dg_confi | |
"repoFullName", repoFullName, | ||
"error", err, | ||
) | ||
return "", nil, nil, fmt.Errorf("failed to load repo cache: %v", err) | ||
return "", nil, nil, nil, fmt.Errorf("failed to load repo cache: %v", err) | ||
} | ||
|
||
var config dg_configuration.DiggerConfig | ||
|
@@ -1122,7 +1131,18 @@ func retrieveConfigFromCache(orgId uint, repoFullName string) (string, *dg_confi | |
"repoFullName", repoFullName, | ||
"error", err, | ||
) | ||
return "", nil, nil, fmt.Errorf("failed to unmarshal config from cache: %v", err) | ||
return "", nil, nil, nil, fmt.Errorf("failed to unmarshal config from cache: %v", err) | ||
} | ||
|
||
var taConfig tac.AtlantisConfig | ||
err = json.Unmarshal(repoCache.TerragruntAtlantisConfig, &taConfig) | ||
if err != nil { | ||
slog.Error("Failed to unmarshal config from cache", | ||
"orgId", orgId, | ||
"repoFullName", repoFullName, | ||
"error", err, | ||
) | ||
return "", nil, nil, nil, fmt.Errorf("failed to unmarshal config from cache: %v", err) | ||
Comment on lines
+1137
to
+1145
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider handling the case where TerragruntAtlantisConfig is nil or empty to avoid unmarshaling errors, similar to how other cached data is validated. |
||
} | ||
|
||
slog.Debug("Creating project dependency graph from cached config", | ||
|
@@ -1138,7 +1158,7 @@ func retrieveConfigFromCache(orgId uint, repoFullName string) (string, *dg_confi | |
"repoFullName", repoFullName, | ||
"error", err, | ||
) | ||
return "", nil, nil, fmt.Errorf("error creating dependency graph from cached config: %v", err) | ||
return "", nil, nil, nil, fmt.Errorf("error creating dependency graph from cached config: %v", err) | ||
} | ||
|
||
slog.Info("Successfully retrieved config from cache", | ||
|
@@ -1147,7 +1167,7 @@ func retrieveConfigFromCache(orgId uint, repoFullName string) (string, *dg_confi | |
"projectCount", len(config.Projects), | ||
) | ||
|
||
return repoCache.DiggerYmlStr, &config, &projectsGraph, nil | ||
return repoCache.DiggerYmlStr, &config, &projectsGraph, &taConfig, nil | ||
} | ||
|
||
func GetRepoByInstllationId(installationId int64, repoOwner string, repoName string) (*models.Repo, error) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
-- Modify "repo_caches" table | ||
ALTER TABLE "public"."repo_caches" ADD COLUMN "terragrunt_atlantis_config" bytea NULL; |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -46,7 +46,7 @@ func main() { | |||||
|
||||||
slog.Info("refreshing projects from repo", "repoFullName", repoFullName) | ||||||
err := utils3.CloneGitRepoAndDoAction(cloneUrl, branch, "", token, "", func(dir string) error { | ||||||
config, err := dg_configuration.LoadDiggerConfigYaml(dir, true, nil) | ||||||
config, _, err := dg_configuration.LoadDiggerConfigYaml(dir, true, nil, nil) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The PR has changed the signature of
Suggested change
|
||||||
if err != nil { | ||||||
slog.Error("failed to load digger.yml: %v", "error", err) | ||||||
return fmt.Errorf("error loading digger.yml %v", err) | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
UpsertRepoCache
function is called withnewAtlantisConfig
which could be nil, as it's returned fromLoadDiggerConfig
. Looking at the migration filebackend/migrations/20250725041417.sql
, we can see that a new columnterragrunt_atlantis_config
was added to therepo_caches
table, which suggests that theUpsertRepoCache
function was updated to handle this new parameter.However, I cannot see the implementation of
UpsertRepoCache
to verify if it properly handles nil values for thenewAtlantisConfig
parameter. IfUpsertRepoCache
doesn't have proper nil checking for this parameter, it could lead to a nil pointer dereference when trying to access or use thenewAtlantisConfig
object.Since I cannot confirm the implementation details of
UpsertRepoCache
, I'm reporting this as a potential issue that should be verified. The code should ensure thatUpsertRepoCache
properly handles nil values for thenewAtlantisConfig
parameter to prevent potential runtime errors.