Surface inaccessible root folders in SourceCrawler status - #304
Surface inaccessible root folders in SourceCrawler status#304gshikhar2021 wants to merge 1 commit into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughThe change tracks inaccessible Google Drive root folders, continues partial crawls, and records folder IDs, URLs, and errors in ChangesRoot folder failure reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SourceCrawlerController
participant GDriveSource
participant GoogleDrive
participant SourceCrawlerStatus
SourceCrawlerController->>GDriveSource: reconcile source
GDriveSource->>GoogleDrive: crawl configured root folders
GoogleDrive-->>GDriveSource: crawl results and failures
GDriveSource-->>SourceCrawlerController: FailedRootFolders and results
SourceCrawlerController->>SourceCrawlerController: map folder IDs to URLs
SourceCrawlerController->>SourceCrawlerStatus: persist inaccessibleRootFolders
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/sourcecrawler_controller.go`:
- Around line 158-161: Update the successMessage construction in the source
crawler reconciliation flow to use singular wording when failedRootFolders
contains exactly one item, while retaining the plural wording for multiple
inaccessible root folders.
- Around line 148-155: Update the error message passed to handleError in the
SyncFilesToFilestore error path to include the inaccessible root count using the
same format as the success path. Include len(failedRootFolders) in the
fmt.Errorf call so the message contains both the storage error and the count of
inaccessible roots, matching the consistency and visibility of the success-path
message construction.
In `@pkg/unstructured/source.go`:
- Around line 294-297: Update the crawl and garbage-collection flow around
FailedRootFolders so files belonging to inaccessible roots are preserved: either
skip garbage collection whenever any root crawl fails, or restrict deletion to
successfully crawled roots. Ensure cached files and permission files from failed
roots are not deleted, and add a regression test covering a cached file from a
failed root.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Enterprise
Run ID: c78c1ca6-476e-48e5-a2aa-33071b9ea5be
📒 Files selected for processing (5)
api/v1alpha1/sourcecrawler_types.goapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/operator.dataverse.redhat.com_sourcecrawlers.yamlinternal/controller/sourcecrawler_controller.gopkg/unstructured/source.go
| if err != nil { | ||
| if err := controllerutils.StatusPatch(ctx, r.Client, sourceCrawlerCR, func() { | ||
| sourceCrawlerCR.Status.InaccessibleRootFolders = failedRootFolders | ||
| }); err != nil { | ||
| logger.Error(err, "failed to update SourceCrawler CR status with inaccessible root folders") | ||
| } | ||
| return ctrl.Result{}, r.handleError(ctx, sourceCrawlerCR, fmt.Errorf("failed to store files to filestore: %w", err)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 'func \(r \*SourceCrawlerReconciler\) handleError|UpdateStatus\(' \
internal/controller/sourcecrawler_controller.goRepository: redhat-data-and-ai/unstructured-data-controller
Length of output: 2707
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, let's see the full function to understand the context
rg -n 'failedRootFolders' internal/controller/sourcecrawler_controller.go
# Also search for where roots are crawled and where errors from that are handled
rg -n 'source\.' internal/controller/sourcecrawler_controller.go | head -20Repository: redhat-data-and-ai/unstructured-data-controller
Length of output: 815
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full Reconcile function to see the exact control flow
sed -n '120,200p' internal/controller/sourcecrawler_controller.goRepository: redhat-data-and-ai/unstructured-data-controller
Length of output: 3920
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let's look at what UpdateStatus does
rg -n 'func.*UpdateStatus' internal/controller/sourcecrawler_controller.go
# Also check if there's a definition in the SourceCrawler type
fd -t f '\.go$' | xargs rg -l 'func.*UpdateStatus' | head -5Repository: redhat-data-and-ai/unstructured-data-controller
Length of output: 185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for UpdateStatus definition across the codebase
rg -n 'UpdateStatus' --type go | grep -E 'func.*UpdateStatus|UpdateStatus\s*\(' | head -20Repository: redhat-data-and-ai/unstructured-data-controller
Length of output: 185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for UpdateStatus definition
rg 'UpdateStatus' --type go -n | head -20Repository: redhat-data-and-ai/unstructured-data-controller
Length of output: 185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all go files and search for UpdateStatus
find . -name "*.go" -type f | xargs grep -n "UpdateStatus" 2>/dev/null | head -30Repository: redhat-data-and-ai/unstructured-data-controller
Length of output: 4885
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the SourceCrawler type definition and its UpdateStatus method
find . -name "*.go" -type f | xargs grep -l "type SourceCrawler" | grep -v vendorRepository: redhat-data-and-ai/unstructured-data-controller
Length of output: 326
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at the SourceCrawler type definition and UpdateStatus method
cat -n api/v1alpha1/sourcecrawler_types.goRepository: redhat-data-and-ai/unstructured-data-controller
Length of output: 6024
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Now let's verify the exact scenario: when SyncFilesToFilestore returns an error,
# are failedRootFolders available and is the count included in the message?
# Looking at lines 141-155 in context:
sed -n '141,170p' internal/controller/sourcecrawler_controller.go
echo "---"
echo "The UpdateStatus method from the types file:"
sed -n '122,145p' api/v1alpha1/sourcecrawler_types.goRepository: redhat-data-and-ai/unstructured-data-controller
Length of output: 2449
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let's trace what happens with failedRootFolders in both paths
# 1. Error path (lines 147-155): failedRootFolders is set in status but not in the error message
# 2. Success path (lines 158-165): failedRootFolders is set in status AND included in the message
# Let's check: in the error path, what message is being passed to handleError?
# Line 154 passes: fmt.Errorf("failed to store files to filestore: %w", err)
# This doesn't include failedRootFolders count
# Let's verify by looking at what message is actually constructed
echo "=== Error path message construction ==="
sed -n '148,155p' internal/controller/sourcecrawler_controller.go
echo ""
echo "=== Success path message construction ==="
sed -n '158,165p' internal/controller/sourcecrawler_controller.go
echo ""
echo "=== What handleError does with the message ==="
sed -n '332,343p' internal/controller/sourcecrawler_controller.goRepository: redhat-data-and-ai/unstructured-data-controller
Length of output: 1720
Include the inaccessible-root count in the error-path status message.
When SyncFilesToFilestore returns an error, failedRootFolders is populated and stored in the CR status. However, the error message passed to handleError at line 154 contains only the storage error, not the count of inaccessible roots. The UpdateStatus method concatenates the message with the error text, so the condition message becomes ", error: ..." without the count.
In contrast, the success path at lines 159–160 explicitly includes the count: fmt.Sprintf(", %d inaccessible root folders", len(failedRootFolders)). For consistency and visibility, pass the count in the error path as well. Modify the message passed to handleError to include len(failedRootFolders) when constructing the error message.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/sourcecrawler_controller.go` around lines 148 - 155,
Update the error message passed to handleError in the SyncFilesToFilestore error
path to include the inaccessible root count using the same format as the success
path. Include len(failedRootFolders) in the fmt.Errorf call so the message
contains both the storage error and the count of inaccessible roots, matching
the consistency and visibility of the success-path message construction.
| successMessage := fmt.Sprintf("successfully reconciled source crawler: %s", sourceCrawlerCR.Name) | ||
| if len(failedRootFolders) > 0 { | ||
| successMessage += fmt.Sprintf(", %d inaccessible root folders", len(failedRootFolders)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use singular wording for one inaccessible root folder.
For one failure, the message is 1 inaccessible root folders. Handle the singular form.
Proposed fix
- successMessage += fmt.Sprintf(", %d inaccessible root folders", len(failedRootFolders))
+ count := len(failedRootFolders)
+ label := "folders"
+ if count == 1 {
+ label = "folder"
+ }
+ successMessage += fmt.Sprintf(", %d inaccessible root %s", count, label)📝 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.
| successMessage := fmt.Sprintf("successfully reconciled source crawler: %s", sourceCrawlerCR.Name) | |
| if len(failedRootFolders) > 0 { | |
| successMessage += fmt.Sprintf(", %d inaccessible root folders", len(failedRootFolders)) | |
| } | |
| successMessage := fmt.Sprintf("successfully reconciled source crawler: %s", sourceCrawlerCR.Name) | |
| if len(failedRootFolders) > 0 { | |
| count := len(failedRootFolders) | |
| label := "folders" | |
| if count == 1 { | |
| label = "folder" | |
| } | |
| successMessage += fmt.Sprintf(", %d inaccessible root %s", count, label) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/sourcecrawler_controller.go` around lines 158 - 161,
Update the successMessage construction in the source crawler reconciliation flow
to use singular wording when failedRootFolders contains exactly one item, while
retaining the plural wording for multiple inaccessible root folders.
| g.FailedRootFolders = append(g.FailedRootFolders, FailedRootFolder{ | ||
| FolderID: g.FolderIDs[i], | ||
| Error: r.err.Error(), | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Prevent garbage collection from deleting files from failed roots.
When one root crawl fails, this code continues with records from successful roots. The later garbage-collection phase builds currentFiles only from those records and deletes local files and permission files that are not present. Cached files from the inaccessible root are therefore deleted during a partial crawl.
Skip garbage collection while any root folder has failed, or limit deletion to roots whose crawls completed successfully. Add a regression test for a cached file from a failed root.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/unstructured/source.go` around lines 294 - 297, Update the crawl and
garbage-collection flow around FailedRootFolders so files belonging to
inaccessible roots are preserved: either skip garbage collection whenever any
root crawl fails, or restrict deletion to successfully crawled roots. Ensure
cached files and permission files from failed roots are not deleted, and add a
regression test covering a cached file from a failed root.
1 participant
When crawling Google Drive folders, service account might not have access to folders mentioned by the user in the unstructured data pipeline
Example status
Summary by CodeRabbit
New Features
Bug Fixes