chore: update crd status for incremental scrape#1986
Open
yashmehrotra wants to merge 1 commit intomainfrom
Open
chore: update crd status for incremental scrape#1986yashmehrotra wants to merge 1 commit intomainfrom
yashmehrotra wants to merge 1 commit intomainfrom
Conversation
Contributor
WalkthroughEnhanced Kubernetes watch consumption flow in the incremental scraper by capturing processing timestamps and adding CRD status update functionality. When processing completes and agent ID is unset, the scraper now patches the CRD status with LastRunStatus containing success state, error details, and execution timestamp. Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scrapers/incremental.go`:
- Around line 201-203: The check for unset agent_id is too strict: in the block
where you read source := sc.ScrapeConfig().GetAnnotations()["source"] and
agentID := sc.ScrapeConfig().GetAnnotations()["agent_id"], replace the single
equality check against uuid.Nil.String() with a check that treats missing/empty
values as unset (e.g., if source == models.SourceCRD && (agentID == "" ||
agentID == uuid.Nil.String())). Update the conditional in the function/method
that contains these variables so CRD status updates run when agent_id is absent
or empty as well as when it equals the nil UUID.
- Around line 201-214: The CRD last-run status (v1.LastRunStatus) is only
patched after consumeResources succeeds, so failures before that never write
LastRunStatus; modify the flow around sc.ScrapeConfig()/jobCtx.History to always
attempt updateCRDStatus(jobCtx.Context, sc.ScrapeConfig(), lastRun) regardless
of consumeResources outcome—e.g., compute lastRun using jobCtx.History and start
time and invoke updateCRDStatus in a defer or in both the success and error
return paths (referencing updateCRDStatus, sc.ScrapeConfig, v1.LastRunStatus,
jobCtx.History); ensure if updateCRDStatus itself fails you surface or combine
that error with the original error (using wrapping or errors.Join) rather than
silently dropping it.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f070578e-0636-4b50-b7a7-b7d2553151ef
📒 Files selected for processing (1)
scrapers/incremental.go
Comment on lines
+201
to
+203
| source := sc.ScrapeConfig().GetAnnotations()["source"] | ||
| agentID := sc.ScrapeConfig().GetAnnotations()["agent_id"] | ||
| if source == models.SourceCRD && agentID == uuid.Nil.String() { |
Contributor
There was a problem hiding this comment.
Unset agent_id check is too strict.
Line 203 only matches the nil UUID literal. If agent_id is absent/empty, this block won’t run and CRD status won’t be updated.
Proposed fix
- if source == models.SourceCRD && agentID == uuid.Nil.String() {
+ if source == models.SourceCRD && (agentID == "" || agentID == uuid.Nil.String()) {📝 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.
Suggested change
| source := sc.ScrapeConfig().GetAnnotations()["source"] | |
| agentID := sc.ScrapeConfig().GetAnnotations()["agent_id"] | |
| if source == models.SourceCRD && agentID == uuid.Nil.String() { | |
| source := sc.ScrapeConfig().GetAnnotations()["source"] | |
| agentID := sc.ScrapeConfig().GetAnnotations()["agent_id"] | |
| if source == models.SourceCRD && (agentID == "" || agentID == uuid.Nil.String()) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scrapers/incremental.go` around lines 201 - 203, The check for unset agent_id
is too strict: in the block where you read source :=
sc.ScrapeConfig().GetAnnotations()["source"] and agentID :=
sc.ScrapeConfig().GetAnnotations()["agent_id"], replace the single equality
check against uuid.Nil.String() with a check that treats missing/empty values as
unset (e.g., if source == models.SourceCRD && (agentID == "" || agentID ==
uuid.Nil.String())). Update the conditional in the function/method that contains
these variables so CRD status updates run when agent_id is absent or empty as
well as when it equals the nil UUID.
Comment on lines
+201
to
+214
| source := sc.ScrapeConfig().GetAnnotations()["source"] | ||
| agentID := sc.ScrapeConfig().GetAnnotations()["agent_id"] | ||
| if source == models.SourceCRD && agentID == uuid.Nil.String() { | ||
| lastRun := v1.LastRunStatus{ | ||
| Success: jobCtx.History.SuccessCount, | ||
| Error: jobCtx.History.ErrorCount, | ||
| Timestamp: metav1.Time{Time: start}, | ||
| Errors: jobCtx.History.Errors, | ||
| } | ||
|
|
||
| if err := updateCRDStatus(jobCtx.Context, sc.ScrapeConfig(), lastRun); err != nil { | ||
| return fmt.Errorf("error patching crd status: %w", err) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Failure runs can exit before CRD status is patched.
This block runs only after successful consumeResources; if processing fails earlier (Line 184), LastRunStatus is never written for that run.
Proposed fix
+ source := sc.ScrapeConfig().GetAnnotations()["source"]
+ agentID := sc.ScrapeConfig().GetAnnotations()["agent_id"]
+ shouldPatchCRDStatus := source == models.SourceCRD && (agentID == "" || agentID == uuid.Nil.String())
+
if err := consumeResources(jobCtx, *sc.ScrapeConfig(), *config, objs, deletedObjects); err != nil {
jobCtx.History.AddErrorf("failed to consume resources: %v", err)
+ if shouldPatchCRDStatus {
+ lastRun := v1.LastRunStatus{
+ Success: jobCtx.History.SuccessCount,
+ Error: jobCtx.History.ErrorCount,
+ Timestamp: metav1.Time{Time: start},
+ Errors: jobCtx.History.Errors,
+ }
+ if patchErr := updateCRDStatus(jobCtx.Context, sc.ScrapeConfig(), lastRun); patchErr != nil {
+ jobCtx.Logger.Errorf("error patching crd status after consume failure: %v", patchErr)
+ }
+ }
return err
}
...
- source := sc.ScrapeConfig().GetAnnotations()["source"]
- agentID := sc.ScrapeConfig().GetAnnotations()["agent_id"]
- if source == models.SourceCRD && agentID == uuid.Nil.String() {
+ if shouldPatchCRDStatus {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scrapers/incremental.go` around lines 201 - 214, The CRD last-run status
(v1.LastRunStatus) is only patched after consumeResources succeeds, so failures
before that never write LastRunStatus; modify the flow around
sc.ScrapeConfig()/jobCtx.History to always attempt
updateCRDStatus(jobCtx.Context, sc.ScrapeConfig(), lastRun) regardless of
consumeResources outcome—e.g., compute lastRun using jobCtx.History and start
time and invoke updateCRDStatus in a defer or in both the success and error
return paths (referencing updateCRDStatus, sc.ScrapeConfig, v1.LastRunStatus,
jobCtx.History); ensure if updateCRDStatus itself fails you surface or combine
that error with the original error (using wrapping or errors.Join) rather than
silently dropping it.
Member
|
@yashmehrotra I think we will need to rate-limit this reduce updates to etcd |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit