From da46c847f73a634bc8025921e159e2444f7bd59e Mon Sep 17 00:00:00 2001 From: Patrick Maslana Date: Tue, 11 Jun 2024 16:02:41 -0700 Subject: [PATCH 1/3] Add more logging to troubleshoot issue and fix the bot names --- internal/github/checkPendingCI.go | 4 +++- k8s/notify-pending-prs.yml.j2 | 2 +- k8s/notify-stale-prs.yml.j2 | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/internal/github/checkPendingCI.go b/internal/github/checkPendingCI.go index 26d458c..aff7636 100644 --- a/internal/github/checkPendingCI.go +++ b/internal/github/checkPendingCI.go @@ -81,7 +81,7 @@ func getLastCommitTime(client *github.Client, owner, repo string, prNumber int) // Since GetDate() returns a Timestamp (not *Timestamp), use the address to call GetTime() commitTime := commitDate.GetTime() // Correctly accessing GetTime(), which returns *time.Time - + log.Printf("The last commit time is %s", commitTime.Format(time.RFC3339)) if commitTime == nil { return time.Time{}, fmt.Errorf("commit time is nil for PR #%d", prNumber) } @@ -103,7 +103,9 @@ func checkTeamMemberActivity(client *github.Client, owner, repo string, prNumber } for _, comment := range comments { + log.Printf("Checking comment by %s at %s", comment.User.GetLogin(), comment.CreatedAt.Format(time.RFC3339)) if _, ok := teamMembers[comment.User.GetLogin()]; ok && comment.CreatedAt.After(lastCommitTime) { + log.Printf("Found team member comment after last commit time: %s", comment.CreatedAt.Format(time.RFC3339)) // Check if the comment is after the last commit return true, nil // Active and relevant participation } diff --git a/k8s/notify-pending-prs.yml.j2 b/k8s/notify-pending-prs.yml.j2 index b7a1f3b..4289e44 100644 --- a/k8s/notify-pending-prs.yml.j2 +++ b/k8s/notify-pending-prs.yml.j2 @@ -5,7 +5,7 @@ image: deployment: args: - - notify-stale + - notify-pendingci - --loop # Creates a secret with the following values, and mounts as a file into the main deployment container diff --git a/k8s/notify-stale-prs.yml.j2 b/k8s/notify-stale-prs.yml.j2 index 4289e44..b7a1f3b 100644 --- a/k8s/notify-stale-prs.yml.j2 +++ b/k8s/notify-stale-prs.yml.j2 @@ -5,7 +5,7 @@ image: deployment: args: - - notify-pendingci + - notify-stale - --loop # Creates a secret with the following values, and mounts as a file into the main deployment container From 9ab34770221cb51dc9affefbb1f4166cd364277b Mon Sep 17 00:00:00 2001 From: Patrick Maslana Date: Wed, 12 Jun 2024 09:00:22 -0700 Subject: [PATCH 2/3] Move a check for the commitTime before trying to print it --- internal/github/checkPendingCI.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/github/checkPendingCI.go b/internal/github/checkPendingCI.go index aff7636..13092fe 100644 --- a/internal/github/checkPendingCI.go +++ b/internal/github/checkPendingCI.go @@ -81,10 +81,11 @@ func getLastCommitTime(client *github.Client, owner, repo string, prNumber int) // Since GetDate() returns a Timestamp (not *Timestamp), use the address to call GetTime() commitTime := commitDate.GetTime() // Correctly accessing GetTime(), which returns *time.Time - log.Printf("The last commit time is %s", commitTime.Format(time.RFC3339)) if commitTime == nil { return time.Time{}, fmt.Errorf("commit time is nil for PR #%d", prNumber) } + log.Printf("The last commit time is %s", commitTime.Format(time.RFC3339)) + return *commitTime, nil // Safely dereference *time.Time to get time.Time } From 3e1a1c80c057ee2759b5d1b40402c89587f3c22f Mon Sep 17 00:00:00 2001 From: Patrick Maslana Date: Wed, 12 Jun 2024 12:03:57 -0700 Subject: [PATCH 3/3] Create contexts and better logging --- cmd/notifyPendingCI.go | 4 +++- cmd/notifyStale.go | 4 +++- internal/github/checkPendingCI.go | 34 ++++++++++++++++--------------- internal/github/checkStalePRs.go | 16 +++++++-------- 4 files changed, 31 insertions(+), 27 deletions(-) diff --git a/cmd/notifyPendingCI.go b/cmd/notifyPendingCI.go index d3784c2..560bd69 100644 --- a/cmd/notifyPendingCI.go +++ b/cmd/notifyPendingCI.go @@ -1,6 +1,7 @@ package cmd import ( + "context" "log" "time" @@ -24,10 +25,11 @@ var notifyPendingCICmd = &cobra.Command{ loop := viper.GetBool("loop") loopDuration := viper.GetDuration("loop-time") + ctx := context.Background() var listPendingPRs []string for { log.Println("Checking for community PRs that are waiting for CI to run") - listPendingPRs, err = github2.CheckForPendingCI(client, cfg) + listPendingPRs, err = github2.CheckForPendingCI(ctx, client, cfg) if err != nil { log.Printf("The following error occurred while obtaining a list of pending PRs: %s", err) time.Sleep(loopDuration) diff --git a/cmd/notifyStale.go b/cmd/notifyStale.go index dbbf3ae..fdc6ca6 100644 --- a/cmd/notifyStale.go +++ b/cmd/notifyStale.go @@ -1,6 +1,7 @@ package cmd import ( + "context" "log" "time" @@ -24,10 +25,11 @@ var notifyStaleCmd = &cobra.Command{ loop := viper.GetBool("loop") loopDuration := viper.GetDuration("loop-time") + ctx := context.Background() var listPendingPRs []string for { log.Println("Checking for community PRs that have no update in the last 7 days") - _, err = github2.CheckStalePRs(client, cfg) + _, err = github2.CheckStalePRs(ctx, client, cfg) if err != nil { log.Printf("The following error occurred while obtaining a list of stale PRs: %s", err) time.Sleep(loopDuration) diff --git a/internal/github/checkPendingCI.go b/internal/github/checkPendingCI.go index 13092fe..0a34d61 100644 --- a/internal/github/checkPendingCI.go +++ b/internal/github/checkPendingCI.go @@ -14,7 +14,7 @@ import ( ) // CheckForPendingCI returns a list of PR URLs that are ready for CI to run but haven't started yet. -func CheckForPendingCI(githubClient *github.Client, cfg *config.Config) ([]string, error) { +func CheckForPendingCI(ctx context.Context, githubClient *github.Client, cfg *config.Config) ([]string, error) { teamMembers, _ := GetTeamMemberList(githubClient, cfg.InternalTeam) var pendingPRs []string @@ -34,8 +34,10 @@ func CheckForPendingCI(githubClient *github.Client, cfg *config.Config) ([]strin } for _, pr := range communityPRs { + ctx, cancel := context.WithTimeout(ctx, 30*time.Second) // 30 seconds timeout for each request + defer cancel() // Dynamic cutoff time based on the last commit to the PR - lastCommitTime, err := getLastCommitTime(githubClient, owner, repo, pr.GetNumber()) + lastCommitTime, err := getLastCommitTime(ctx, githubClient, owner, repo, pr.GetNumber()) if err != nil { log.Printf("Error retrieving last commit time for PR #%d in %s/%s: %v", pr.GetNumber(), owner, repo, err) continue @@ -47,13 +49,13 @@ func CheckForPendingCI(githubClient *github.Client, cfg *config.Config) ([]strin continue } - hasCIRuns, err := checkCIStatus(githubClient, owner, repo, pr.GetNumber()) + hasCIRuns, err := checkCIStatus(ctx, githubClient, owner, repo, pr.GetNumber()) if err != nil { log.Printf("Error checking CI status for PR #%d in %s/%s: %v", pr.GetNumber(), owner, repo, err) continue } - teamMemberActivity, err := checkTeamMemberActivity(githubClient, owner, repo, pr.GetNumber(), teamMembers, lastCommitTime) + teamMemberActivity, err := checkTeamMemberActivity(ctx, githubClient, owner, repo, pr.GetNumber(), teamMembers, lastCommitTime) if err != nil { log.Printf("Error checking team member activity for PR #%d in %s/%s: %v", pr.GetNumber(), owner, repo, err) continue // or handle the error as needed @@ -67,13 +69,13 @@ func CheckForPendingCI(githubClient *github.Client, cfg *config.Config) ([]strin return pendingPRs, nil } -func getLastCommitTime(client *github.Client, owner, repo string, prNumber int) (time.Time, error) { - commits, _, err := client.PullRequests.ListCommits(context.Background(), owner, repo, prNumber, nil) +func getLastCommitTime(ctx context.Context, client *github.Client, owner, repo string, prNumber int) (time.Time, error) { + commits, _, err := client.PullRequests.ListCommits(ctx, owner, repo, prNumber, nil) if err != nil { return time.Time{}, err // Properly handle API errors } if len(commits) == 0 { - return time.Time{}, fmt.Errorf("no commits found for PR #%d", prNumber) // Handle case where no commits are found + return time.Time{}, fmt.Errorf("no commits found for PR #%d of repo %s", prNumber, repo) // Handle case where no commits are found } // Requesting a list of commits will return the json list in descending order lastCommit := commits[len(commits)-1] @@ -82,31 +84,31 @@ func getLastCommitTime(client *github.Client, owner, repo string, prNumber int) // Since GetDate() returns a Timestamp (not *Timestamp), use the address to call GetTime() commitTime := commitDate.GetTime() // Correctly accessing GetTime(), which returns *time.Time if commitTime == nil { - return time.Time{}, fmt.Errorf("commit time is nil for PR #%d", prNumber) + return time.Time{}, fmt.Errorf("commit time is nil for PR #%d of repo %s", prNumber, repo) } - log.Printf("The last commit time is %s", commitTime.Format(time.RFC3339)) + log.Printf("The last commit time is %s for PR #%d of repo %s", commitTime.Format(time.RFC3339), prNumber, repo) return *commitTime, nil // Safely dereference *time.Time to get time.Time } -func checkCIStatus(client *github.Client, owner, repo string, prNumber int) (bool, error) { - checks, _, err := client.Checks.ListCheckRunsForRef(context.Background(), owner, repo, strconv.Itoa(prNumber), &github.ListCheckRunsOptions{}) +func checkCIStatus(ctx context.Context, client *github.Client, owner, repo string, prNumber int) (bool, error) { + checks, _, err := client.Checks.ListCheckRunsForRef(ctx, owner, repo, strconv.Itoa(prNumber), &github.ListCheckRunsOptions{}) if err != nil { return false, err } return checks.GetTotal() > 0, nil } -func checkTeamMemberActivity(client *github.Client, owner, repo string, prNumber int, teamMembers map[string]bool, lastCommitTime time.Time) (bool, error) { - comments, _, err := client.Issues.ListComments(context.Background(), owner, repo, prNumber, nil) +func checkTeamMemberActivity(ctx context.Context, client *github.Client, owner, repo string, prNumber int, teamMembers map[string]bool, lastCommitTime time.Time) (bool, error) { + comments, _, err := client.Issues.ListComments(ctx, owner, repo, prNumber, nil) if err != nil { return false, fmt.Errorf("failed to fetch comments: %w", err) } for _, comment := range comments { - log.Printf("Checking comment by %s at %s", comment.User.GetLogin(), comment.CreatedAt.Format(time.RFC3339)) + log.Printf("Checking comment by %s at %s for PR #%d of repo %s", comment.User.GetLogin(), comment.CreatedAt.Format(time.RFC3339), prNumber, repo) if _, ok := teamMembers[comment.User.GetLogin()]; ok && comment.CreatedAt.After(lastCommitTime) { - log.Printf("Found team member comment after last commit time: %s", comment.CreatedAt.Format(time.RFC3339)) + log.Printf("Found team member comment after last commit time: %s for PR #%d of repo %s", comment.CreatedAt.Format(time.RFC3339), prNumber, repo) // Check if the comment is after the last commit return true, nil // Active and relevant participation } @@ -114,7 +116,7 @@ func checkTeamMemberActivity(client *github.Client, owner, repo string, prNumber reviews, _, err := client.PullRequests.ListReviews(context.Background(), owner, repo, prNumber, nil) if err != nil { - return false, fmt.Errorf("failed to fetch reviews: %w", err) + return false, fmt.Errorf("failed to fetch reviews: %w for PR #%d of repo %s", err, prNumber, repo) } for _, review := range reviews { diff --git a/internal/github/checkStalePRs.go b/internal/github/checkStalePRs.go index 455fde9..c08203d 100644 --- a/internal/github/checkStalePRs.go +++ b/internal/github/checkStalePRs.go @@ -12,7 +12,7 @@ import ( ) // CheckStalePRs will return a list of PR URLs that have not been updated in the last 7 days by internal team members. -func CheckStalePRs(githubClient *github.Client, cfg *config.Config) ([]string, error) { +func CheckStalePRs(ctx context.Context, githubClient *github.Client, cfg *config.Config) ([]string, error) { var stalePRUrls []string cutoffDate := time.Now().Add(7 * 24 * time.Hour) // 7 days ago teamMembers, err := GetTeamMemberList(githubClient, cfg.InternalTeam) @@ -25,8 +25,8 @@ func CheckStalePRs(githubClient *github.Client, cfg *config.Config) ([]string, e } for _, pr := range communityPRs { - repoName := pr.GetBase().GetRepo().GetFullName() // Get the full name of the repository - stale, err := isStale(githubClient, pr, teamMembers, cutoffDate) // Handle both returned values + repoName := pr.GetBase().GetRepo().GetFullName() // Get the full name of the repository + stale, err := isStale(ctx, githubClient, pr, teamMembers, cutoffDate) // Handle both returned values if err != nil { log.Printf("Error checking if PR in repo %s is stale: %v", repoName, err) continue // Skip this PR or handle the error appropriately @@ -39,23 +39,21 @@ func CheckStalePRs(githubClient *github.Client, cfg *config.Config) ([]string, e } // Checks if a PR is stale based on the last update from team members -func isStale(githubClient *github.Client, pr *github.PullRequest, teamMembers map[string]bool, cutoffDate time.Time) (bool, error) { +func isStale(ctx context.Context, githubClient *github.Client, pr *github.PullRequest, teamMembers map[string]bool, cutoffDate time.Time) (bool, error) { listOptions := &github.ListOptions{PerPage: 100} for { // Create a context for each request - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) // 30 seconds timeout for each request - + ctx, cancel := context.WithTimeout(ctx, 30*time.Second) // 30 seconds timeout for each request + defer cancel() events, resp, err := githubClient.Issues.ListIssueTimeline(ctx, pr.Base.Repo.Owner.GetLogin(), pr.Base.Repo.GetName(), pr.GetNumber(), listOptions) if err != nil { - cancel() // Explicitly cancel the context when an error occurs - return false, fmt.Errorf("failed to get timeline for PR #%d: %w", pr.GetNumber(), err) + return false, fmt.Errorf("failed to get timeline for PR #%d of repo %s: %w", pr.GetNumber(), pr.Base.Repo.GetName(), err) } for _, event := range events { if event.Event == nil || event.Actor == nil || event.Actor.Login == nil { continue } if (*event.Event == "commented" || *event.Event == "reviewed") && teamMembers[*event.Actor.Login] && event.CreatedAt.After(cutoffDate) { - cancel() // Clean up the context when returning within the loop return false, nil } }