ensure link to workflow url always present #1892
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here: app.greptile.com/review/github.
💡 (5/5) You can turn off certain types of comments like style here!
11 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
WalkthroughThe changes introduce a new method Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant CiBackend
participant Database
participant GitHubService
Scheduler->>CiBackend: GetWorkflowUrl(spec)
alt Workflow URL unavailable
loop (up to 30 attempts)
CiBackend-->>Scheduler: Error or empty URL
Scheduler->>Scheduler: Wait 1 second
end
else Workflow URL obtained
Scheduler->>Database: Update job with URL
Database-->>Scheduler: Confirmation/Error
Scheduler->>GitHubService: Update issue comment
end
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 3
🧹 Nitpick comments (7)
ee/backend/ci_backends/gitlab_pipeline.go (1)
63-65: Implement or document the placeholder methodThe method is currently a placeholder that returns empty string and nil error. Consider either implementing the actual logic to retrieve a GitLab pipeline URL or adding a TODO comment to indicate this is planned for future implementation.
func (g GitlabPipelineCI) GetWorkflowUrl(spec spec.Spec) (string, error) { + // TODO: Implement GitLab workflow URL retrieval logic return "", nil }ee/backend/ci_backends/bitbucket_pipeline.go (1)
44-46: Add implementation or TODO note to the placeholder methodThe method documentation is helpful, but the actual implementation is missing. Consider either implementing the logic to retrieve a Bitbucket pipeline URL or adding a TODO comment to clearly indicate this is planned for future implementation.
func (bbp BitbucketPipelineCI) GetWorkflowUrl(spec spec.Spec) (string, error) { + // TODO: Implement Bitbucket workflow URL retrieval logic return "", nil }backend/tasks/runs_test.go (1)
31-33: Consider enhancing the mock implementation for testingThe current mock implementation is basic. For more robust testing, consider making it configurable to return different URLs or errors for different test scenarios.
func (m MockCiBackend) GetWorkflowUrl(spec spec.Spec) (string, error) { - return "", nil + // This could be expanded to support test scenarios like: + // - return configurable URLs for different specs + // - simulate errors for specific cases + return "https://mock-ci.example.com/workflow/123", nil }backend/ci_backends/github_actions.go (1)
35-43: Simplify error handling and add success loggingThe implementation can be simplified by removing the unnecessary
elseblock. Consider also adding success logging for better traceability.func (g GithubActionCi) GetWorkflowUrl(spec spec.Spec) (string, error) { _, workflowRunUrl, err := utils.GetWorkflowIdAndUrlFromDiggerJobId(g.Client, spec.VCS.RepoOwner, spec.VCS.RepoName, spec.JobId) if err != nil { log.Printf("Error getting workflow ID from job: %v", err) return "", err - } else { - return workflowRunUrl, nil } + log.Printf("Successfully retrieved workflow URL: %s for job: %s", workflowRunUrl, spec.JobId) + return workflowRunUrl, nil }ee/backend/controllers/bitbucket.go (1)
143-151: Panic recovery block is a robust approach, but consider reducing duplicate logs.
Catching and logging panics in this handler prevents the entire server from crashing, which is good. However, the same panic message is logged multiple times (lines 145–149). To avoid confusion and verbosity, consider merging them into a single structured log message.backend/services/scheduler.go (2)
138-139: Asynchronous call toUpdateWorkflowUrlForJobis promising.
Spawning this function in a separate goroutine helps avoid blocking. Just be mindful of error handling in asynchronous contexts, as logs may be the only visibility mechanism.
161-176: Polling approach is straightforward, consider an exponential backoff.
Your loop attempts to fetch the workflow URL every second for 30 seconds. This may suffice, but an exponential backoff strategy (e.g., increasing sleeps) can reduce load on external services.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/ci_backends/ci_backends.go(1 hunks)backend/ci_backends/github_actions.go(2 hunks)backend/services/scheduler.go(2 hunks)backend/tasks/runs_test.go(1 hunks)backend/utils/github.go(0 hunks)ee/backend/ci_backends/bitbucket_pipeline.go(1 hunks)ee/backend/ci_backends/buildkite.go(1 hunks)ee/backend/ci_backends/gitlab_pipeline.go(1 hunks)ee/backend/controllers/bitbucket.go(2 hunks)next/ci_backends/ci_backends.go(1 hunks)next/ci_backends/github_actions.go(1 hunks)
💤 Files with no reviewable changes (1)
- backend/utils/github.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
backend/services/scheduler.go
191-191: ineffectual assignment to err
(ineffassign)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (8)
next/ci_backends/ci_backends.go (1)
10-10: Interface extension looks good.The addition of the
GetWorkflowUrlmethod to theCiBackendinterface is a logical extension to support retrieving workflow URLs, which aligns with the PR summary's objective of refreshing PR summaries from backend systems.backend/ci_backends/ci_backends.go (1)
10-10: Consistent interface implementation across codebase.The addition of the
GetWorkflowUrlmethod here matches the same change in thenext/ci_backends/ci_backends.gofile, maintaining consistency across the codebase.next/ci_backends/github_actions.go (1)
34-36: Placeholder implementation needs completion.This is currently a stub implementation of the
GetWorkflowUrlmethod for GitHub Actions. Since the PR goal is to refresh PR summaries from the backend, you'll need to implement this to actually fetch the workflow URL from GitHub.Consider using the GitHub API to retrieve the workflow run URL. Based on the existing
TriggerWorkflowimplementation, you might want to use something like:func (g GithubActionCi) GetWorkflowUrl(spec spec.Spec) (string, error) { - return "", nil + client := g.Client + // Get workflow runs for the repository + runs, _, err := client.Actions.ListRepositoryWorkflowRuns( + context.Background(), + spec.VCS.RepoOwner, + spec.VCS.RepoName, + &github.ListWorkflowRunsOptions{ + Branch: &spec.Job.Branch, + }, + ) + if err != nil { + return "", err + } + + // Find the relevant workflow run and return its URL + // This is a simplified example, you'll need to determine how to identify the correct run + if len(runs.WorkflowRuns) > 0 { + return *runs.WorkflowRuns[0].HTMLURL, nil + } + + return "", nil }Also check if a utility function similar to the one mentioned in the AI summary (
utils.GetWorkflowIdAndUrlFromDiggerJobId) already exists that you could leverage.ee/backend/controllers/bitbucket.go (1)
27-27: Import ofruntime/debuglooks good.
This import is used in your new panic recovery logic. It's helpful for gathering stack traces in the event of unexpected runtime panics.backend/services/scheduler.go (4)
9-11: New imports align well with usage.
These additional imports forcomment_updater,orchestrator_scheduler, andspecare consistent with the new asynchronous workflow URL fetching and comment-updating features.
15-16:runtime/debugandtimeimports are appropriate.
runtime/debugsupports your panic recovery stack traces, whiletimeprovides delays for polling the workflow URL. Both are valid additions.
143-154: Panic recovery ensures resilience.
Your deferred function that recovers from panics and logs stack traces prevents a crash if an unexpected error occurs. This is a recommended safeguard for background operations.
192-202: Ensure post-update comment logic is robust.
After fetching the workflow URL and updating the batch, you repeatedly continue the loop on errors (lines 193, 195, 199, 201). Confirm whether these errors would merit exiting earlier or retrying with a backoff to avoid unnecessary repeated attempts.
| func (b BuildkiteCi) GetWorkflowUrl(spec spec.Spec) (string, error) { | ||
| return "", nil | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Placeholder implementation needs completion.
This is currently a stub implementation of the GetWorkflowUrl method for Buildkite CI integration. As this PR is a work in progress, ensure that the actual implementation is completed before finalizing to properly retrieve Buildkite workflow URLs.
🏁 Script executed:
#!/bin/bash
# Check if there are any other implementations of GetWorkflowUrl in the codebase that might serve as a reference
rg "func \(.+\) GetWorkflowUrl" -A 10 --type go | grep -v "return \"\", nil"Length of output: 1926
Attention: Complete Buildkite Workflow URL Implementation
The current implementation of BuildkiteCi.GetWorkflowUrl in ee/backend/ci_backends/buildkite.go (lines 35–37) remains a stub that simply returns an empty string. Unlike the more detailed implementation in the GitHub Actions CI integration—which retrieves the URL using external API calls—the Buildkite implementation still needs to fetch the actual workflow URL and handle errors appropriately.
- Location to address:
ee/backend/ci_backends/buildkite.go, lines 35–37. - Action required: Replace the placeholder code with a complete implementation that retrieves Buildkite workflow URLs, following best practices similar to those seen in other CI integrations.
| func (g GithubActionCi) GetWorkflowUrl(spec spec.Spec) (string, error) { | ||
| _, workflowRunUrl, err := utils.GetWorkflowIdAndUrlFromDiggerJobId(g.Client, spec.VCS.RepoOwner, spec.VCS.RepoName, spec.JobId) | ||
| if err != nil { | ||
| log.Printf("Error getting workflow ID from job: %v", err) | ||
| return "", err | ||
| } else { | ||
| return workflowRunUrl, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for JobId before fetching workflow URL
The current implementation assumes that spec.JobId is always provided and valid. Consider adding validation to handle cases where it might be empty.
func (g GithubActionCi) GetWorkflowUrl(spec spec.Spec) (string, error) {
+ if spec.JobId == "" {
+ log.Printf("Cannot get workflow URL: JobId is empty")
+ return "", fmt.Errorf("job ID is required to fetch workflow URL")
+ }
+
_, workflowRunUrl, err := utils.GetWorkflowIdAndUrlFromDiggerJobId(g.Client, spec.VCS.RepoOwner, spec.VCS.RepoName, spec.JobId)
if err != nil {
log.Printf("Error getting workflow ID from job: %v", err)
return "", err
} else {
return workflowRunUrl, nil
}
}Don't forget to add the import for fmt if it's not already present.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (g GithubActionCi) GetWorkflowUrl(spec spec.Spec) (string, error) { | |
| _, workflowRunUrl, err := utils.GetWorkflowIdAndUrlFromDiggerJobId(g.Client, spec.VCS.RepoOwner, spec.VCS.RepoName, spec.JobId) | |
| if err != nil { | |
| log.Printf("Error getting workflow ID from job: %v", err) | |
| return "", err | |
| } else { | |
| return workflowRunUrl, nil | |
| } | |
| } | |
| func (g GithubActionCi) GetWorkflowUrl(spec spec.Spec) (string, error) { | |
| if spec.JobId == "" { | |
| log.Printf("Cannot get workflow URL: JobId is empty") | |
| return "", fmt.Errorf("job ID is required to fetch workflow URL") | |
| } | |
| _, workflowRunUrl, err := utils.GetWorkflowIdAndUrlFromDiggerJobId(g.Client, spec.VCS.RepoOwner, spec.VCS.RepoName, spec.JobId) | |
| if err != nil { | |
| log.Printf("Error getting workflow ID from job: %v", err) | |
| return "", err | |
| } else { | |
| return workflowRunUrl, nil | |
| } | |
| } |
backend/services/scheduler.go
Outdated
| continue | ||
| } | ||
| // TODO: make this abstract and extracting the right "prService" based on VCS | ||
| client, _, err := utils.GetGithubService(gh, batch.GithubInstallationId, spec.VCS.RepoFullname, spec.VCS.RepoOwner, spec.VCS.RepoName) |
There was a problem hiding this comment.
Address ineffectual assignment to err.
At line 191, err is assigned from GetGithubService but immediately overwritten at line 192 without being checked. This leads to a warning from static analysis and may conceal important errors.
Below is a possible fix that checks for the error before overwriting it:
- client, _, err := utils.GetGithubService(gh, batch.GithubInstallationId, spec.VCS.RepoFullname, spec.VCS.RepoOwner, spec.VCS.RepoName)
- err = comment_updater.BasicCommentUpdater{}.UpdateComment(res.Jobs, *spec.Job.PullRequestNumber, client, spec.CommentId)
+ client, _, fetchErr := utils.GetGithubService(gh, batch.GithubInstallationId, spec.VCS.RepoFullname, spec.VCS.RepoOwner, spec.VCS.RepoName)
+ if fetchErr != nil {
+ log.Printf("DiggerJobId %v: Error getting GitHub service: %v", job.DiggerJobID, fetchErr)
+ continue
+ }
+ err = comment_updater.BasicCommentUpdater{}.UpdateComment(res.Jobs, *spec.Job.PullRequestNumber, client, spec.CommentId)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| client, _, err := utils.GetGithubService(gh, batch.GithubInstallationId, spec.VCS.RepoFullname, spec.VCS.RepoOwner, spec.VCS.RepoName) | |
| client, _, fetchErr := utils.GetGithubService(gh, batch.GithubInstallationId, spec.VCS.RepoFullname, spec.VCS.RepoOwner, spec.VCS.RepoName) | |
| if fetchErr != nil { | |
| log.Printf("DiggerJobId %v: Error getting GitHub service: %v", job.DiggerJobID, fetchErr) | |
| continue | |
| } | |
| err = comment_updater.BasicCommentUpdater{}.UpdateComment(res.Jobs, *spec.Job.PullRequestNumber, client, spec.CommentId) |
🧰 Tools
🪛 golangci-lint (1.62.2)
191-191: ineffectual assignment to err
(ineffassign)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
backend/controllers/projects.go (1)
451-467: Good implementation of workflow URL update, but consider potential race conditions.The implementation correctly retrieves and updates the workflow run URL for a job when the job status is updated. The conditional check on line 460 ensures that only valid URLs (not "#" or empty strings) are stored.
However, this code might run concurrently with the asynchronous
UpdateWorkflowUrlForJobfunction in the scheduler service, potentially creating a race condition for updating the same field.Consider adding a check to only update the URL if it's not already set:
- } else if workflowRunUrl != "#" && workflowRunUrl != "" { + } else if workflowRunUrl != "#" && workflowRunUrl != "" && (job.WorkflowRunUrl == nil || *job.WorkflowRunUrl == "" || *job.WorkflowRunUrl == "#") {This would avoid overwriting a valid URL that might have been set by the async function.
backend/services/scheduler.go (3)
154-180: Consider making the retry mechanism configurable.The function has a good retry mechanism with reasonable delays, but the 30 attempts with 1-second intervals are hardcoded. This might not be optimal for all environments or CI systems.
Consider making these values configurable through environment variables or configuration settings:
+ maxRetries := config.DiggerConfig.GetInt("workflow_url_max_retries") + if maxRetries == 0 { + maxRetries = 30 // Default value + } + retryDelay := time.Duration(config.DiggerConfig.GetInt("workflow_url_retry_delay")) * time.Millisecond + if retryDelay == 0 { + retryDelay = 1 * time.Second // Default value + } - for n := 0; n < 30; n++ { - time.Sleep(1 * time.Second) + for n := 0; n < maxRetries; n++ { + time.Sleep(retryDelay)
181-183: Consider adding final error log for workflow URL retrieval failure.The comment indicates that if execution reaches this point, the workflow likely failed to start, but there's no log message or action taken.
Add a final log message to clearly indicate that all attempts to retrieve the workflow URL failed:
// if we get to here its highly likely that the workflow job entirely failed to start for some reason + log.Printf("DiggerJobId %v: Failed to retrieve workflow URL after multiple attempts. The workflow may have failed to start.", job.DiggerJobID)
161-179: Add exponential backoff for more efficient retries.The current implementation uses a constant 1-second delay between retries. An exponential backoff strategy would be more efficient and reduce unnecessary API calls when the workflow takes longer to initialize.
Consider implementing an exponential backoff:
+ baseDelay := 1 * time.Second for n := 0; n < 30; n++ { - time.Sleep(1 * time.Second) + // Exponential backoff with a cap of 10 seconds + delay := time.Duration(min(float64(baseDelay) * math.Pow(1.5, float64(n)), 10 * float64(time.Second))) + time.Sleep(delay) workflowUrl, err := ciBackend.GetWorkflowUrl(*spec) if err != nil { log.Printf("DiggerJobId %v: error while attempting to fetch workflow url: %v", job.DiggerJobID, err) } else { if workflowUrl == "#" || workflowUrl == "" { log.Printf("DiggerJobId %v: got blank workflow url as response, ignoring", job.DiggerJobID) } else { job.WorkflowRunUrl = &workflowUrl err = models.DB.UpdateDiggerJob(job) if err != nil { log.Printf("DiggerJobId %v: Error updating digger job: %v", job.DiggerJobID, err) continue } else { log.Printf("DiggerJobId %v: successfully updated workflow run url to: %v for DiggerJobID: %v", job.DiggerJobID, workflowUrl, job.DiggerJobID) } return } } }Also, you'll need to add the math package to your imports:
import ( "math" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/controllers/projects.go(1 hunks)backend/services/scheduler.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (3)
backend/services/scheduler.go (3)
10-15: Well-organized imports for new functionality.The additional imports are properly organized and necessary for the new asynchronous URL update functionality.
137-137: Good use of goroutine for non-blocking workflow URL updates.Using a goroutine to call
UpdateWorkflowUrlForJobis appropriate as it allows the job triggering process to complete without waiting for the workflow URL to be available, which might take some time.
142-153: Robust panic recovery implementation in asynchronous function.The panic recovery is well-implemented with detailed logging of both the error and stack trace, which will be valuable for debugging.
motatoes
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/ci_backends/github_actions.go (1)
36-49: Implementation of GetWorkflowUrl looks good, with minor style suggestion.The method correctly validates the JobId parameter and properly handles errors when fetching the workflow URL. This implementation aligns with the PR objective to ensure workflow URLs are always present.
The
elseblock is unnecessary since the function returns in the precedingifblock. Consider simplifying:if err != nil { log.Printf("Error getting workflow ID from job: %v", err) return "", err -} else { - return workflowRunUrl, nil } +return workflowRunUrl, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/ci_backends/github_actions.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
Summary by CodeRabbit
New Features
Bug Fixes