From 72eda882e5eee7f395d2de4ba601e92226215444 Mon Sep 17 00:00:00 2001 From: Hannes Gustafsson Date: Thu, 24 Oct 2024 14:45:51 +0100 Subject: [PATCH 1/8] Use separate context for setting the commit status When the event handling takes to long the previous context is canceled and the request fails and the commit status ends up in a pending state. Using a separate context will allow the status to always be set, regardless of the event handling timing out. --- internal/pkg/githubapi/github.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index 338c591..b7e8f3c 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -836,7 +836,7 @@ func (p *GhPrClientDetails) ToggleCommitStatus(context string, user string) erro func SetCommitStatus(ghPrClientDetails GhPrClientDetails, state string) { // TODO change all these values - context := "telefonistka" + tcontext := "telefonistka" avatarURL := "https://avatars.githubusercontent.com/u/1616153?s=64" description := "Telefonistka GitOps Bot" targetURL := commitStatusTargetURL(time.Now()) @@ -845,11 +845,17 @@ func SetCommitStatus(ghPrClientDetails GhPrClientDetails, state string) { TargetURL: &targetURL, Description: &description, State: &state, - Context: &context, + Context: &tcontext, AvatarURL: &avatarURL, } ghPrClientDetails.PrLogger.Debugf("Setting commit %s status to %s", ghPrClientDetails.PrSHA, state) - _, resp, err := ghPrClientDetails.GhClientPair.v3Client.Repositories.CreateStatus(ghPrClientDetails.Ctx, ghPrClientDetails.Owner, ghPrClientDetails.Repo, ghPrClientDetails.PrSHA, commitStatus) + + // use a separate context to avoid event processing timeout to cause + // failures in updating the commit status + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + + _, resp, err := ghPrClientDetails.GhClientPair.v3Client.Repositories.CreateStatus(ctx, ghPrClientDetails.Owner, ghPrClientDetails.Repo, ghPrClientDetails.PrSHA, commitStatus) prom.InstrumentGhCall(resp) if err != nil { ghPrClientDetails.PrLogger.Errorf("Failed to set commit status: err=%s\n%v", err, resp) From bc1468b45a50bcfca0207895a6262ddd10ae5a51 Mon Sep 17 00:00:00 2001 From: Hannes Gustafsson Date: Mon, 28 Oct 2024 08:10:57 +0000 Subject: [PATCH 2/8] Simplify label check function Since it is very specific, might as well make it operate directly on labels. This should make it slightly clearer and easier to read. This reverts commit fcd2aeffd5b2752c7274514b7c78be7fc1bc60fd. --- internal/pkg/githubapi/github.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index b7e8f3c..db7b77a 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -197,7 +197,7 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr } // If the PR is a promotion PR and the diff is empty, we can auto-merge it // "len(componentPathList) > 0" validates we are not auto-merging a PR that we failed to understand which apps it affects - if DoesPrHasLabel(*eventPayload, "promotion") && config.Argocd.AutoMergeNoDiffPRs && len(componentPathList) > 0 { + if DoesPrHasLabel(eventPayload.PullRequest.Labels, "promotion") && config.Argocd.AutoMergeNoDiffPRs && len(componentPathList) > 0 { ghPrClientDetails.PrLogger.Infof("Auto-merging (no diff) PR %d", *eventPayload.PullRequest.Number) err := MergePr(ghPrClientDetails, eventPayload.PullRequest.Number) if err != nil { @@ -238,7 +238,7 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr prHandleError = err ghPrClientDetails.PrLogger.Errorf("Drift detection failed: err=%s\n", err) } - } else if *eventPayload.Action == "labeled" && DoesPrHasLabel(*eventPayload, "show-plan") { + } else if *eventPayload.Action == "labeled" && DoesPrHasLabel(eventPayload.PullRequest.Labels, "show-plan") { SetCommitStatus(ghPrClientDetails, "pending") wasCommitStatusSet = true ghPrClientDetails.PrLogger.Infoln("Found show-plan label, posting plan") @@ -784,15 +784,13 @@ func (p GhPrClientDetails) CommentOnPr(commentBody string) error { return err } -func DoesPrHasLabel(eventPayload github.PullRequestEvent, name string) bool { - result := false - for _, prLabel := range eventPayload.PullRequest.Labels { - if *prLabel.Name == name { - result = true - break +func DoesPrHasLabel(labels []*github.Label, name string) bool { + for _, l := range labels { + if *l.Name == name { + return true } } - return result + return false } func (p *GhPrClientDetails) ToggleCommitStatus(context string, user string) error { From 7f8fda7f4cf7778aca8cfee49a37ffdc175d6649 Mon Sep 17 00:00:00 2001 From: Hannes Gustafsson Date: Mon, 28 Oct 2024 08:12:56 +0000 Subject: [PATCH 3/8] Refactor to recover from panic in event handlers This change refactors the event handling logic such that a deferred panic handler can log panics in the downstream handler logic. This should avoid crashing when such panics occur, and instead it would log the panic using the logger. Additionally, the parsing of the event payload to determine which handling logic to invoke is separated out, and now also indicates whether a match was found. This is to allow PR status updates to be applied once, when appropriate, and to enable ensuring that the success/failure update is always applied. To achieve this the individual downstream logic is factored out into separate functions, and errors encountered in them are returned where prHandleError were previously set. Getting the default branch and Telefonistka config is duplicated in each handler as needed. --- internal/pkg/githubapi/github.go | 223 +++++++++++++++++-------------- 1 file changed, 126 insertions(+), 97 deletions(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index db7b77a..a6dbf9a 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -127,132 +127,161 @@ func shouldSyncBranchCheckBoxBeDisplayed(componentPathList []string, allowSyncfr } func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPrClientDetails, mainGithubClientPair GhClientPair, approverGithubClientPair GhClientPair, ctx context.Context) { + defer func() { + if r := recover(); r != nil { + ghPrClientDetails.PrLogger.Errorf("Recovered: %v", r) + } + }() + ghPrClientDetails.getPrMetadata(eventPayload.PullRequest.GetBody()) - // wasCommitStatusSet and the placement of SetCommitStatus in the flow is used to ensure an API call is only made where it needed - wasCommitStatusSet := false - var prHandleError error + stat, ok := eventToHandle(eventPayload) + if !ok { + // nothing to do + return + } + + SetCommitStatus(ghPrClientDetails, "pending") + + var err error + + defer func() { + if err != nil { + SetCommitStatus(ghPrClientDetails, "error") + return + } + SetCommitStatus(ghPrClientDetails, "success") + }() + + switch stat { + case "merged": + err = handleMergedPrEvent(ghPrClientDetails, approverGithubClientPair.v3Client) + case "changed": + err = handleChangedPREvent(ctx, mainGithubClientPair, ghPrClientDetails, eventPayload) + case "show-plan": + err = handleShowPlanPREvent(ctx, ghPrClientDetails, eventPayload) + } + if err != nil { + ghPrClientDetails.PrLogger.Errorf("Handling of PR event failed: err=%s\n", err) + } +} + +// eventToHandle returns the event to be handled, translated from a Github +// world into the Telefonistka world. If no event should be handled, ok is +// false. +func eventToHandle(eventPayload *github.PullRequestEvent) (event string, ok bool) { + switch { + case *eventPayload.Action == "closed" && *eventPayload.PullRequest.Merged: + return "merged", true + case *eventPayload.Action == "opened" || *eventPayload.Action == "reopened" || *eventPayload.Action == "synchronize": + return "changed", true + case *eventPayload.Action == "labeled" && DoesPrHasLabel(eventPayload.PullRequest.Labels, "show-plan"): + return "show-plan", true + default: + return "", false + } +} + +func handleShowPlanPREvent(ctx context.Context, ghPrClientDetails GhPrClientDetails, eventPayload *github.PullRequestEvent) error { + ghPrClientDetails.PrLogger.Infoln("Found show-plan label, posting plan") defaultBranch, _ := ghPrClientDetails.GetDefaultBranch() config, err := GetInRepoConfig(ghPrClientDetails, defaultBranch) if err != nil { ghPrClientDetails.PrLogger.Infof("Couldn't get Telefonistka in-repo configuration: %v", err) } + promotions, _ := GeneratePromotionPlan(ghPrClientDetails, config, *eventPayload.PullRequest.Head.Ref) + commentPlanInPR(ghPrClientDetails, promotions) + return nil +} - if *eventPayload.Action == "closed" && *eventPayload.PullRequest.Merged { - SetCommitStatus(ghPrClientDetails, "pending") - wasCommitStatusSet = true - err := handleMergedPrEvent(ghPrClientDetails, approverGithubClientPair.v3Client) - if err != nil { - prHandleError = err - ghPrClientDetails.PrLogger.Errorf("Handling of merged PR failed: err=%s\n", err) - } - } else if *eventPayload.Action == "opened" || *eventPayload.Action == "reopened" || *eventPayload.Action == "synchronize" { - SetCommitStatus(ghPrClientDetails, "pending") - wasCommitStatusSet = true - botIdentity, _ := GetBotGhIdentity(mainGithubClientPair.v4Client, ctx) - err = MimizeStalePrComments(ghPrClientDetails, mainGithubClientPair.v4Client, botIdentity) +func handleChangedPREvent(ctx context.Context, mainGithubClientPair GhClientPair, ghPrClientDetails GhPrClientDetails, eventPayload *github.PullRequestEvent) error { + botIdentity, _ := GetBotGhIdentity(mainGithubClientPair.v4Client, ctx) + err := MimizeStalePrComments(ghPrClientDetails, mainGithubClientPair.v4Client, botIdentity) + if err != nil { + return fmt.Errorf("minimizing stale PR comments: %w", err) + } + defaultBranch, _ := ghPrClientDetails.GetDefaultBranch() + config, err := GetInRepoConfig(ghPrClientDetails, defaultBranch) + if err != nil { + ghPrClientDetails.PrLogger.Infof("Couldn't get Telefonistka in-repo configuration: %v", err) + } + if config.Argocd.CommentDiffonPR { + componentPathList, err := generateListOfChangedComponentPaths(ghPrClientDetails, config) if err != nil { - prHandleError = err - ghPrClientDetails.PrLogger.Errorf("Failed to minimize stale comments: err=%s\n", err) + return fmt.Errorf("generate list of changed components: %w", err) } - if config.Argocd.CommentDiffonPR { - componentPathList, err := generateListOfChangedComponentPaths(ghPrClientDetails, config) + + // Building a map component's path and a boolean value that indicates if we should diff it not. + // I'm avoiding doing this in the ArgoCD package to avoid circular dependencies and keep package scope clean + componentsToDiff := map[string]bool{} + for _, componentPath := range componentPathList { + c, err := getComponentConfig(ghPrClientDetails, componentPath, ghPrClientDetails.Ref) if err != nil { - prHandleError = err - ghPrClientDetails.PrLogger.Errorf("Failed to get list of changed components: err=%s\n", err) + return fmt.Errorf("get component (%s) config: %w", componentPath, err) + } else { + if c.DisableArgoCDDiff { + componentsToDiff[componentPath] = false + ghPrClientDetails.PrLogger.Debugf("ArgoCD diff disabled for %s\n", componentPath) + } else { + componentsToDiff[componentPath] = true + } } - - // Building a map component's path and a boolean value that indicates if we should diff it not. - // I'm avoiding doing this in the ArgoCD package to avoid circular dependencies and keep package scope clean - componentsToDiff := map[string]bool{} - for _, componentPath := range componentPathList { - c, err := getComponentConfig(ghPrClientDetails, componentPath, ghPrClientDetails.Ref) + } + hasComponentDiff, hasComponentDiffErrors, diffOfChangedComponents, err := argocd.GenerateDiffOfChangedComponents(ctx, componentsToDiff, ghPrClientDetails.Ref, ghPrClientDetails.RepoURL, config.Argocd.UseSHALabelForAppDiscovery, config.Argocd.CreateTempAppObjectFroNewApps) + if err != nil { + return fmt.Errorf("getting diff information: %w", err) + } else { + ghPrClientDetails.PrLogger.Debugf("Successfully got ArgoCD diff(comparing live objects against objects rendered form git ref %s)", ghPrClientDetails.Ref) + if !hasComponentDiffErrors && !hasComponentDiff { + ghPrClientDetails.PrLogger.Debugf("ArgoCD diff is empty, this PR will not change cluster state\n") + prLables, resp, err := ghPrClientDetails.GhClientPair.v3Client.Issues.AddLabelsToIssue(ghPrClientDetails.Ctx, ghPrClientDetails.Owner, ghPrClientDetails.Repo, *eventPayload.PullRequest.Number, []string{"noop"}) + prom.InstrumentGhCall(resp) if err != nil { - prHandleError = fmt.Errorf("Failed to get component config(%s): err=%s\n", componentPath, err) - ghPrClientDetails.PrLogger.Error(prHandleError) + ghPrClientDetails.PrLogger.Errorf("Could not label GitHub PR: err=%s\n%v\n", err, resp) } else { - if c.DisableArgoCDDiff { - componentsToDiff[componentPath] = false - ghPrClientDetails.PrLogger.Debugf("ArgoCD diff disabled for %s\n", componentPath) - } else { - componentsToDiff[componentPath] = true - } + ghPrClientDetails.PrLogger.Debugf("PR %v labeled\n%+v", *eventPayload.PullRequest.Number, prLables) } - } - hasComponentDiff, hasComponentDiffErrors, diffOfChangedComponents, err := argocd.GenerateDiffOfChangedComponents(ctx, componentsToDiff, ghPrClientDetails.Ref, ghPrClientDetails.RepoURL, config.Argocd.UseSHALabelForAppDiscovery, config.Argocd.CreateTempAppObjectFroNewApps) - if err != nil { - prHandleError = err - ghPrClientDetails.PrLogger.Errorf("Failed to get ArgoCD diff information: err=%s\n", err) - } else { - ghPrClientDetails.PrLogger.Debugf("Successfully got ArgoCD diff(comparing live objects against objects rendered form git ref %s)", ghPrClientDetails.Ref) - if !hasComponentDiffErrors && !hasComponentDiff { - ghPrClientDetails.PrLogger.Debugf("ArgoCD diff is empty, this PR will not change cluster state\n") - prLables, resp, err := ghPrClientDetails.GhClientPair.v3Client.Issues.AddLabelsToIssue(ghPrClientDetails.Ctx, ghPrClientDetails.Owner, ghPrClientDetails.Repo, *eventPayload.PullRequest.Number, []string{"noop"}) - prom.InstrumentGhCall(resp) + // If the PR is a promotion PR and the diff is empty, we can auto-merge it + // "len(componentPathList) > 0" validates we are not auto-merging a PR that we failed to understand which apps it affects + if DoesPrHasLabel(eventPayload.PullRequest.Labels, "promotion") && config.Argocd.AutoMergeNoDiffPRs && len(componentPathList) > 0 { + ghPrClientDetails.PrLogger.Infof("Auto-merging (no diff) PR %d", *eventPayload.PullRequest.Number) + err := MergePr(ghPrClientDetails, eventPayload.PullRequest.Number) if err != nil { - ghPrClientDetails.PrLogger.Errorf("Could not label GitHub PR: err=%s\n%v\n", err, resp) - } else { - ghPrClientDetails.PrLogger.Debugf("PR %v labeled\n%+v", *eventPayload.PullRequest.Number, prLables) - } - // If the PR is a promotion PR and the diff is empty, we can auto-merge it - // "len(componentPathList) > 0" validates we are not auto-merging a PR that we failed to understand which apps it affects - if DoesPrHasLabel(eventPayload.PullRequest.Labels, "promotion") && config.Argocd.AutoMergeNoDiffPRs && len(componentPathList) > 0 { - ghPrClientDetails.PrLogger.Infof("Auto-merging (no diff) PR %d", *eventPayload.PullRequest.Number) - err := MergePr(ghPrClientDetails, eventPayload.PullRequest.Number) - if err != nil { - prHandleError = err - ghPrClientDetails.PrLogger.Errorf("PR auto merge failed: err=%v", err) - } + return fmt.Errorf("PR auto merge: %w", err) } } } + } - if len(diffOfChangedComponents) > 0 { - diffCommentData := DiffCommentData{ - DiffOfChangedComponents: diffOfChangedComponents, - BranchName: ghPrClientDetails.Ref, - } + if len(diffOfChangedComponents) > 0 { + diffCommentData := DiffCommentData{ + DiffOfChangedComponents: diffOfChangedComponents, + BranchName: ghPrClientDetails.Ref, + } - diffCommentData.DisplaySyncBranchCheckBox = shouldSyncBranchCheckBoxBeDisplayed(componentPathList, config.Argocd.AllowSyncfromBranchPathRegex, diffOfChangedComponents) + diffCommentData.DisplaySyncBranchCheckBox = shouldSyncBranchCheckBoxBeDisplayed(componentPathList, config.Argocd.AllowSyncfromBranchPathRegex, diffOfChangedComponents) - comments, err := generateArgoCdDiffComments(diffCommentData, githubCommentMaxSize) + comments, err := generateArgoCdDiffComments(diffCommentData, githubCommentMaxSize) + if err != nil { + return fmt.Errorf("generate diff comment: %w", err) + } + for _, comment := range comments { + err = commentPR(ghPrClientDetails, comment) if err != nil { - prHandleError = err - ghPrClientDetails.PrLogger.Errorf("Failed to comment ArgoCD diff: err=%s\n", err) + return fmt.Errorf("commenting on PR: %w", err) } - for _, comment := range comments { - err = commentPR(ghPrClientDetails, comment) - if err != nil { - prHandleError = err - ghPrClientDetails.PrLogger.Errorf("Failed to comment on PR: err=%s\n", err) - } - } - } else { - ghPrClientDetails.PrLogger.Debugf("Diff not find affected ArogCD apps") } - } - ghPrClientDetails.PrLogger.Infoln("Checking for Drift") - err = DetectDrift(ghPrClientDetails) - if err != nil { - prHandleError = err - ghPrClientDetails.PrLogger.Errorf("Drift detection failed: err=%s\n", err) - } - } else if *eventPayload.Action == "labeled" && DoesPrHasLabel(eventPayload.PullRequest.Labels, "show-plan") { - SetCommitStatus(ghPrClientDetails, "pending") - wasCommitStatusSet = true - ghPrClientDetails.PrLogger.Infoln("Found show-plan label, posting plan") - promotions, _ := GeneratePromotionPlan(ghPrClientDetails, config, *eventPayload.PullRequest.Head.Ref) - commentPlanInPR(ghPrClientDetails, promotions) - } - - if wasCommitStatusSet { - if prHandleError == nil { - SetCommitStatus(ghPrClientDetails, "success") } else { - SetCommitStatus(ghPrClientDetails, "error") + ghPrClientDetails.PrLogger.Debugf("Diff not find affected ArogCD apps") } } + ghPrClientDetails.PrLogger.Infoln("Checking for Drift") + err = DetectDrift(ghPrClientDetails) + if err != nil { + return fmt.Errorf("detecting drift: %w", err) + } + return nil } func generateArgoCdDiffComments(diffCommentData DiffCommentData, githubCommentMaxSize int) (comments []string, err error) { From b411c14407838102aec376d476e7de8280c64d72 Mon Sep 17 00:00:00 2001 From: Hannes Gustafsson Date: Mon, 28 Oct 2024 08:33:36 +0000 Subject: [PATCH 4/8] Move log statement and change from info to debug The message seems to be informative only to developers i.e. better suited as a debug message. Moving it to the function that it is actually logging for ensures that it is always logged, instead of putting this burden on the caller. --- internal/pkg/githubapi/github.go | 1 - internal/pkg/githubapi/promotion.go | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index a6dbf9a..011f0a1 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -276,7 +276,6 @@ func handleChangedPREvent(ctx context.Context, mainGithubClientPair GhClientPair ghPrClientDetails.PrLogger.Debugf("Diff not find affected ArogCD apps") } } - ghPrClientDetails.PrLogger.Infoln("Checking for Drift") err = DetectDrift(ghPrClientDetails) if err != nil { return fmt.Errorf("detecting drift: %w", err) diff --git a/internal/pkg/githubapi/promotion.go b/internal/pkg/githubapi/promotion.go index aac4edb..4d5bbba 100644 --- a/internal/pkg/githubapi/promotion.go +++ b/internal/pkg/githubapi/promotion.go @@ -51,6 +51,7 @@ func contains(s []string, str string) bool { } func DetectDrift(ghPrClientDetails GhPrClientDetails) error { + ghPrClientDetails.PrLogger.Debugln("Checking for Drift") if ghPrClientDetails.Ctx.Err() != nil { return ghPrClientDetails.Ctx.Err() } From c7b3259276b0d10f1355569f55dc4108c70213a0 Mon Sep 17 00:00:00 2001 From: Hannes Gustafsson Date: Mon, 28 Oct 2024 08:36:34 +0000 Subject: [PATCH 5/8] Drop else statements no longer needed Now that the earlier error is returned, the else is not needed and can be dropped, reducing the indentation of the happy path [1, 2]. [1] https://maelvls.dev/go-happy-line-of-sight/ [2] https://medium.com/@matryer/line-of-sight-in-code-186dd7cdea88 --- internal/pkg/githubapi/github.go | 47 +++++++++++++++----------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index 011f0a1..fe7e2df 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -219,37 +219,34 @@ func handleChangedPREvent(ctx context.Context, mainGithubClientPair GhClientPair c, err := getComponentConfig(ghPrClientDetails, componentPath, ghPrClientDetails.Ref) if err != nil { return fmt.Errorf("get component (%s) config: %w", componentPath, err) - } else { - if c.DisableArgoCDDiff { - componentsToDiff[componentPath] = false - ghPrClientDetails.PrLogger.Debugf("ArgoCD diff disabled for %s\n", componentPath) - } else { - componentsToDiff[componentPath] = true - } + } + componentsToDiff[componentPath] = true + if c.DisableArgoCDDiff { + componentsToDiff[componentPath] = false + ghPrClientDetails.PrLogger.Debugf("ArgoCD diff disabled for %s\n", componentPath) } } hasComponentDiff, hasComponentDiffErrors, diffOfChangedComponents, err := argocd.GenerateDiffOfChangedComponents(ctx, componentsToDiff, ghPrClientDetails.Ref, ghPrClientDetails.RepoURL, config.Argocd.UseSHALabelForAppDiscovery, config.Argocd.CreateTempAppObjectFroNewApps) if err != nil { return fmt.Errorf("getting diff information: %w", err) - } else { - ghPrClientDetails.PrLogger.Debugf("Successfully got ArgoCD diff(comparing live objects against objects rendered form git ref %s)", ghPrClientDetails.Ref) - if !hasComponentDiffErrors && !hasComponentDiff { - ghPrClientDetails.PrLogger.Debugf("ArgoCD diff is empty, this PR will not change cluster state\n") - prLables, resp, err := ghPrClientDetails.GhClientPair.v3Client.Issues.AddLabelsToIssue(ghPrClientDetails.Ctx, ghPrClientDetails.Owner, ghPrClientDetails.Repo, *eventPayload.PullRequest.Number, []string{"noop"}) - prom.InstrumentGhCall(resp) + } + ghPrClientDetails.PrLogger.Debugf("Successfully got ArgoCD diff(comparing live objects against objects rendered form git ref %s)", ghPrClientDetails.Ref) + if !hasComponentDiffErrors && !hasComponentDiff { + ghPrClientDetails.PrLogger.Debugf("ArgoCD diff is empty, this PR will not change cluster state\n") + prLables, resp, err := ghPrClientDetails.GhClientPair.v3Client.Issues.AddLabelsToIssue(ghPrClientDetails.Ctx, ghPrClientDetails.Owner, ghPrClientDetails.Repo, *eventPayload.PullRequest.Number, []string{"noop"}) + prom.InstrumentGhCall(resp) + if err != nil { + ghPrClientDetails.PrLogger.Errorf("Could not label GitHub PR: err=%s\n%v\n", err, resp) + } else { + ghPrClientDetails.PrLogger.Debugf("PR %v labeled\n%+v", *eventPayload.PullRequest.Number, prLables) + } + // If the PR is a promotion PR and the diff is empty, we can auto-merge it + // "len(componentPathList) > 0" validates we are not auto-merging a PR that we failed to understand which apps it affects + if DoesPrHasLabel(eventPayload.PullRequest.Labels, "promotion") && config.Argocd.AutoMergeNoDiffPRs && len(componentPathList) > 0 { + ghPrClientDetails.PrLogger.Infof("Auto-merging (no diff) PR %d", *eventPayload.PullRequest.Number) + err := MergePr(ghPrClientDetails, eventPayload.PullRequest.Number) if err != nil { - ghPrClientDetails.PrLogger.Errorf("Could not label GitHub PR: err=%s\n%v\n", err, resp) - } else { - ghPrClientDetails.PrLogger.Debugf("PR %v labeled\n%+v", *eventPayload.PullRequest.Number, prLables) - } - // If the PR is a promotion PR and the diff is empty, we can auto-merge it - // "len(componentPathList) > 0" validates we are not auto-merging a PR that we failed to understand which apps it affects - if DoesPrHasLabel(eventPayload.PullRequest.Labels, "promotion") && config.Argocd.AutoMergeNoDiffPRs && len(componentPathList) > 0 { - ghPrClientDetails.PrLogger.Infof("Auto-merging (no diff) PR %d", *eventPayload.PullRequest.Number) - err := MergePr(ghPrClientDetails, eventPayload.PullRequest.Number) - if err != nil { - return fmt.Errorf("PR auto merge: %w", err) - } + return fmt.Errorf("PR auto merge: %w", err) } } } From e31bf467c802cd8c169db7e5386df91f4ec1b1a5 Mon Sep 17 00:00:00 2001 From: Hannes Gustafsson Date: Mon, 28 Oct 2024 11:19:50 +0000 Subject: [PATCH 6/8] Adjust some logging statements * Change metric type log level to debug Logging metric style info is better to handle properly; since this is one of very few instances it is changed to debug level for now. Future goal is to add tracing and metric instrumentation for such information. * Log event type once at start of handle function * Drop now duplicate log lines of the event type * Consistently add event type into PR logger Once PR logger includes the event_type. For consistency add the same field to the other PR loggers. * Remove now unused eventType argument --- internal/pkg/argocd/argocd.go | 2 +- internal/pkg/githubapi/github.go | 21 +++++++++++---------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index ae63f20..3d3175f 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -277,7 +277,7 @@ func findArgocdAppByManifestPathAnnotation(ctx context.Context, componentPath st getAppsStart := time.Now() allRepoApps, err := appClient.List(ctx, &appQuery) getAppsDuration := time.Since(getAppsStart).Milliseconds() - log.Infof("Got %v ArgoCD applications for repo %s in %v ms", len(allRepoApps.Items), repo, getAppsDuration) + log.Debugf("Got %v ArgoCD applications for repo %s in %v ms", len(allRepoApps.Items), repo, getAppsDuration) if err != nil { return nil, err } diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index fe7e2df..c96f842 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -341,7 +341,7 @@ func ReciveEventFile(eventType string, eventFilePath string, mainGhClientCache * r.Header.Set("Content-Type", "application/json") r.Header.Set("X-GitHub-Event", eventType) - handleEvent(eventPayloadInterface, mainGhClientCache, prApproverGhClientCache, r, payload, eventType) + handleEvent(eventPayloadInterface, mainGhClientCache, prApproverGhClientCache, r, payload) } // ReciveWebhook is the main entry point for the webhook handling it starts parases the webhook payload and start a thread to handle the event success/failure are dependant on the payload parsing only @@ -362,11 +362,11 @@ func ReciveWebhook(r *http.Request, mainGhClientCache *lru.Cache[string, GhClien } prom.InstrumentWebhookHit("successful") - go handleEvent(eventPayloadInterface, mainGhClientCache, prApproverGhClientCache, r, payload, eventType) + go handleEvent(eventPayloadInterface, mainGhClientCache, prApproverGhClientCache, r, payload) return nil } -func handleEvent(eventPayloadInterface interface{}, mainGhClientCache *lru.Cache[string, GhClientPair], prApproverGhClientCache *lru.Cache[string, GhClientPair], r *http.Request, payload []byte, eventType string) { +func handleEvent(eventPayloadInterface interface{}, mainGhClientCache *lru.Cache[string, GhClientPair], prApproverGhClientCache *lru.Cache[string, GhClientPair], r *http.Request, payload []byte) { // We don't use the request context as it might have a short deadline and we don't want to stop event handling based on that // But we do want to stop the event handling after a certain point, so: ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) @@ -374,10 +374,11 @@ func handleEvent(eventPayloadInterface interface{}, mainGhClientCache *lru.Cache var mainGithubClientPair GhClientPair var approverGithubClientPair GhClientPair + log.Infof("Handling event type %T", eventPayloadInterface) + switch eventPayload := eventPayloadInterface.(type) { case *github.PushEvent: // this is a commit push, do something with it? - log.Infoln("is PushEvent") repoOwner := *eventPayload.Repo.Owner.Login mainGithubClientPair.GetAndCache(mainGhClientCache, "GITHUB_APP_ID", "GITHUB_APP_PRIVATE_KEY_PATH", "GITHUB_OAUTH_TOKEN", repoOwner, ctx) @@ -399,8 +400,9 @@ func handleEvent(eventPayloadInterface interface{}, mainGhClientCache *lru.Cache log.Infof("is PullRequestEvent(%s)", *eventPayload.Action) prLogger := log.WithFields(log.Fields{ - "repo": *eventPayload.Repo.Owner.Login + "/" + *eventPayload.Repo.Name, - "prNumber": *eventPayload.PullRequest.Number, + "repo": *eventPayload.Repo.Owner.Login + "/" + *eventPayload.Repo.Name, + "prNumber": *eventPayload.PullRequest.Number, + "event_type": "pr", }) repoOwner := *eventPayload.Repo.Owner.Login @@ -429,10 +431,10 @@ func handleEvent(eventPayloadInterface interface{}, mainGhClientCache *lru.Cache mainGithubClientPair.GetAndCache(mainGhClientCache, "GITHUB_APP_ID", "GITHUB_APP_PRIVATE_KEY_PATH", "GITHUB_OAUTH_TOKEN", repoOwner, ctx) botIdentity, _ := GetBotGhIdentity(mainGithubClientPair.v4Client, ctx) - log.Infof("Actionable event type %s\n", eventType) prLogger := log.WithFields(log.Fields{ - "repo": *eventPayload.Repo.Owner.Login + "/" + *eventPayload.Repo.Name, - "prNumber": *eventPayload.Issue.Number, + "repo": *eventPayload.Repo.Owner.Login + "/" + *eventPayload.Repo.Name, + "prNumber": *eventPayload.Issue.Number, + "event_type": "issue_comment", }) // Ignore comment events sent by the bot (this is about who trigger the event not who wrote the comment) if *eventPayload.Sender.Login != botIdentity { @@ -452,7 +454,6 @@ func handleEvent(eventPayloadInterface interface{}, mainGhClientCache *lru.Cache } default: - log.Infof("Non-actionable event type %s", eventType) return } } From 596bd89817d0d3880a69f8a113659b6223035487 Mon Sep 17 00:00:00 2001 From: Hannes Gustafsson Date: Mon, 28 Oct 2024 11:28:52 +0000 Subject: [PATCH 7/8] Return error when unable to read configuration Prior when the configuration was not successfully fetched, the error was only logged but execution continued. Not fetching the configuration is an unrecoverable error that should result in upstream failure. Instead return the error to caller and let them log it. --- internal/pkg/githubapi/github.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index c96f842..a6cb0ea 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -188,7 +188,7 @@ func handleShowPlanPREvent(ctx context.Context, ghPrClientDetails GhPrClientDeta defaultBranch, _ := ghPrClientDetails.GetDefaultBranch() config, err := GetInRepoConfig(ghPrClientDetails, defaultBranch) if err != nil { - ghPrClientDetails.PrLogger.Infof("Couldn't get Telefonistka in-repo configuration: %v", err) + return fmt.Errorf("get in-repo configuration: %w", err) } promotions, _ := GeneratePromotionPlan(ghPrClientDetails, config, *eventPayload.PullRequest.Head.Ref) commentPlanInPR(ghPrClientDetails, promotions) @@ -204,7 +204,7 @@ func handleChangedPREvent(ctx context.Context, mainGithubClientPair GhClientPair defaultBranch, _ := ghPrClientDetails.GetDefaultBranch() config, err := GetInRepoConfig(ghPrClientDetails, defaultBranch) if err != nil { - ghPrClientDetails.PrLogger.Infof("Couldn't get Telefonistka in-repo configuration: %v", err) + return fmt.Errorf("get in-repo configuration: %w", err) } if config.Argocd.CommentDiffonPR { componentPathList, err := generateListOfChangedComponentPaths(ghPrClientDetails, config) From b17f800bbe737194d8c4fcbf60f5c5c715a2f50a Mon Sep 17 00:00:00 2001 From: Hannes Gustafsson Date: Mon, 28 Oct 2024 15:38:02 +0000 Subject: [PATCH 8/8] Remove dead code --- internal/pkg/githubapi/clients.go | 6 ------ internal/pkg/githubapi/github.go | 23 ----------------------- 2 files changed, 29 deletions(-) diff --git a/internal/pkg/githubapi/clients.go b/internal/pkg/githubapi/clients.go index a0bb9cb..33f2ea3 100644 --- a/internal/pkg/githubapi/clients.go +++ b/internal/pkg/githubapi/clients.go @@ -132,7 +132,6 @@ func createGithubGraphQlClient(githubOauthToken string, githubGraphqlAltURL stri } func createGhAppClientPair(ctx context.Context, githubAppId int64, owner string, ghAppPKeyPathEnvVarName string) GhClientPair { - // var ghClientPair *GhClientPair var githubRestAltURL string var githubGraphqlAltURL string githubAppPrivateKeyPath := getCrucialEnv(ghAppPKeyPathEnvVarName) @@ -151,8 +150,6 @@ func createGhAppClientPair(ctx context.Context, githubAppId int64, owner string, log.Errorf("Couldn't find installation for app ID %v and repo owner %s", githubAppId, owner) } - // ghClientPair.v3Client := createGithubAppRestClient(githubAppPrivateKeyPath, githubAppId, githubAppInstallationId, githubRestAltURL, ctx) - // ghClientPair.v4Client := createGithubAppGraphQlClient(githubAppPrivateKeyPath, githubAppId, githubAppInstallationId, githubGraphqlAltURL, githubRestAltURL, ctx) return GhClientPair{ v3Client: createGithubAppRestClient(githubAppPrivateKeyPath, githubAppId, githubAppInstallationId, githubRestAltURL, ctx), v4Client: createGithubAppGraphQlClient(githubAppPrivateKeyPath, githubAppId, githubAppInstallationId, githubGraphqlAltURL, githubRestAltURL, ctx), @@ -160,7 +157,6 @@ func createGhAppClientPair(ctx context.Context, githubAppId int64, owner string, } func createGhTokenClientPair(ctx context.Context, ghOauthToken string) GhClientPair { - // var ghClientPair *GhClientPair var githubRestAltURL string var githubGraphqlAltURL string githubHost := getEnv("GITHUB_HOST", "") @@ -173,8 +169,6 @@ func createGhTokenClientPair(ctx context.Context, ghOauthToken string) GhClientP log.Debugf("Using public Github API endpoint") } - // ghClientPair.v3Client := createGithubRestClient(ghOauthToken, githubRestAltURL, ctx) - // ghClientPair.v4Client := createGithubGraphQlClient(ghOauthToken, githubGraphqlAltURL) return GhClientPair{ v3Client: createGithubRestClient(ghOauthToken, githubRestAltURL, ctx), v4Client: createGithubGraphQlClient(ghOauthToken, githubGraphqlAltURL), diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index a6cb0ea..4d50998 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -75,11 +75,6 @@ func (pm prMetadata) serialize() (string, error) { if err != nil { return "", err } - // var compressedPmJson []byte - // _, err = lz4.CompressBlock(pmJson, compressedPmJson, nil) - // if err != nil { - // return "", err - // } return base64.StdEncoding.EncodeToString(pmJson), nil } @@ -624,7 +619,6 @@ func handleMergedPrEvent(ghPrClientDetails GhPrClientDetails, prApproverGithubCl // configBranch = default branch as the PR is closed at this and its branch deleted. // If we'l ever want to generate this plan on an unmerged PR the PR branch (ghPrClientDetails.Ref) should be used promotions, _ := GeneratePromotionPlan(ghPrClientDetails, config, defaultBranch) - // log.Infof("%+v", promotions) if !config.DryRunMode { for _, promotion := range promotions { // TODO this whole part shouldn't be in main, but I need to refactor some circular dep's @@ -790,10 +784,6 @@ func (pm *prMetadata) DeSerialize(s string) error { if err != nil { return err } - // _, err = lz4.UncompressBlock(decoded, unCompressedPmJson) - // if err != nil { - // return err - // } err = json.Unmarshal(decoded, pm) return err } @@ -1225,19 +1215,6 @@ func createPrObject(ghPrClientDetails GhPrClientDetails, newBranchRef string, ne ghPrClientDetails.PrLogger.Debugf(" %s was set as assignee on PR", assignee) } - // reviewers := github.ReviewersRequest{ - // Reviewers: []string{"SA-k8s-pr-approver-bot"}, // TODO remove hardcoding - // } - // - // _, resp, err = ghPrClientDetails.Ghclient.PullRequests.RequestReviewers(ghPrClientDetails.Ctx, ghPrClientDetails.Owner, ghPrClientDetails.Repo, *pull.Number, reviewers) - // prom.InstrumentGhCall(resp) - // if err != nil { - // ghPrClientDetails.PrLogger.Errorf("Could not set reviewer on pr: err=%s\n%v\n", err, resp) - // return pull, err - // } else { - // ghPrClientDetails.PrLogger.Debugf("PR reviewer set.\n%+v", reviewers) - // } - return pull, nil // TODO }