fix(upstream): update config last scraped time id for agent#1877
Merged
fix(upstream): update config last scraped time id for agent#1877
Conversation
Benchstat (Other)Base: ✅ No significant performance changes detectedFull benchstat output |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
Benchstat (RLS)Base: 📊 5 minor regression(s) (all within 5% threshold)
✅ 2 improvement(s)
Full benchstat output |
![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
🧹 Nitpick comments (1)
upstream/upstream_test.go (1)
1-2: Drop theABOUTMEbanner comments.These top-level comments are boilerplate and don’t add actionable context in this test file.
As per coding guidelines,
**/*.go: Only add comments if really necessary; do not add comments that simply explain the code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upstream/upstream_test.go` around lines 1 - 2, Remove the top-of-file boilerplate banner comments labeled "ABOUTME" from upstream_test.go; open the test file (upstream_test.go), delete the two ABOUTME comment lines at the top so the file starts with package declaration or actual test code, and ensure no other non-actionable comments remain—no code changes required to functions like Test*; just remove the banner comments to comply with the comment guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@upstream/upstream_test.go`:
- Around line 15-86: TestAddAgentConfig uses native t.Errorf/t.Fatalf
assertions; replace them with Gomega by creating g := gomega.NewWithT(t) at
start of the test and convert each assertion to g.Expect(...) style—e.g., check
lengths with g.Expect(len(pushData.ConfigItems)).To(Equal(1)), equality checks
for IDs/names with g.Expect(...).To(Equal(...)), and ensure boolean conditions
use appropriate matchers. Also add the github.com/onsi/gomega import and remove
direct t.Fatalf/t.Errorf calls, keeping the rest of the test logic (references:
TestAddAgentConfig, pushData.AddAgentConfig, pushData.ConfigItems,
pushData.ConfigChanges, pushData.ConfigScrapers,
pushData.ConfigItemsLastScrapedTime).
In `@upstream/upstream.go`:
- Around line 357-362: Only remap nil ConfigID entries in
t.ConfigItemsLastScrapedTime when there is actually an agent config item in
t.ConfigItems; before the loop over t.ConfigItemsLastScrapedTime, scan
t.ConfigItems for a config item matching the current agent (e.g. check for
ci.AgentID == agent.ID or whichever field links config items to the agent) and
set a boolean like hasAgentConfig; then perform the existing remap
(t.ConfigItemsLastScrapedTime[i].ConfigID = agent.ID) only if hasAgentConfig is
true, leaving entries untouched otherwise to avoid creating dangling config_id
references.
---
Nitpick comments:
In `@upstream/upstream_test.go`:
- Around line 1-2: Remove the top-of-file boilerplate banner comments labeled
"ABOUTME" from upstream_test.go; open the test file (upstream_test.go), delete
the two ABOUTME comment lines at the top so the file starts with package
declaration or actual test code, and ensure no other non-actionable comments
remain—no code changes required to functions like Test*; just remove the banner
comments to comply with the comment guidelines.
🪄 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: 1e7b93e9-c70e-46cb-8c76-dbe6495f3c41
📒 Files selected for processing (2)
upstream/upstream.goupstream/upstream_test.go
Comment on lines
+15
to
+86
| func TestAddAgentConfig(t *testing.T) { | ||
| agentID := uuid.New() | ||
| agent := models.Agent{ | ||
| ID: agentID, | ||
| Name: "test-agent", | ||
| } | ||
|
|
||
| now := time.Now() | ||
|
|
||
| pushData := &PushData{ | ||
| ConfigItems: []models.ConfigItem{ | ||
| { | ||
| ID: uuid.Nil, | ||
| Type: lo.ToPtr("MissionControl::ShouldBeFiltered"), | ||
| }, | ||
| { | ||
| ID: uuid.New(), | ||
| Type: lo.ToPtr("MissionControl::Agent"), | ||
| }, | ||
| }, | ||
| ConfigChanges: []models.ConfigChange{ | ||
| {ConfigID: uuid.Nil.String()}, | ||
| }, | ||
| ConfigScrapers: []models.ConfigScraper{ | ||
| {ID: uuid.Nil}, | ||
| {ID: uuid.New()}, | ||
| }, | ||
| ConfigItemsLastScrapedTime: []models.ConfigItemLastScrapedTime{ | ||
| {ConfigID: uuid.Nil, LastScrapedTime: &now}, | ||
| }, | ||
| } | ||
|
|
||
| pushData.AddAgentConfig(agent) | ||
|
|
||
| // Config item with uuid.Nil ID should be filtered out | ||
| if len(pushData.ConfigItems) != 1 { | ||
| t.Fatalf("expected 1 config item, got %d", len(pushData.ConfigItems)) | ||
| } | ||
|
|
||
| // MissionControl::Agent item should have ID set to agent ID | ||
| ci := pushData.ConfigItems[0] | ||
| if ci.ID != agentID { | ||
| t.Errorf("expected agent config item ID to be %s, got %s", agentID, ci.ID) | ||
| } | ||
| if lo.FromPtr(ci.Name) != "test-agent" { | ||
| t.Errorf("expected agent config item name to be 'test-agent', got %s", lo.FromPtr(ci.Name)) | ||
| } | ||
| if lo.FromPtr(ci.ScraperID) != uuid.Nil.String() { | ||
| t.Errorf("expected scraper_id to be nil UUID, got %s", lo.FromPtr(ci.ScraperID)) | ||
| } | ||
|
|
||
| // Config changes with uuid.Nil config_id should be remapped | ||
| if pushData.ConfigChanges[0].ConfigID != agentID.String() { | ||
| t.Errorf("expected config change config_id to be %s, got %s", agentID, pushData.ConfigChanges[0].ConfigID) | ||
| } | ||
|
|
||
| // System scraper should be filtered out | ||
| if len(pushData.ConfigScrapers) != 1 { | ||
| t.Fatalf("expected 1 config scraper, got %d", len(pushData.ConfigScrapers)) | ||
| } | ||
| if pushData.ConfigScrapers[0].ID == uuid.Nil { | ||
| t.Error("system scraper should have been filtered out") | ||
| } | ||
|
|
||
| // Last scraped time with uuid.Nil config_id should be remapped to agent ID | ||
| if len(pushData.ConfigItemsLastScrapedTime) != 1 { | ||
| t.Fatalf("expected 1 last scraped time entry, got %d", len(pushData.ConfigItemsLastScrapedTime)) | ||
| } | ||
| if pushData.ConfigItemsLastScrapedTime[0].ConfigID != agentID { | ||
| t.Errorf("expected last scraped time config_id to be %s, got %s", agentID, pushData.ConfigItemsLastScrapedTime[0].ConfigID) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use Gomega assertions in this test.
Please migrate these assertions to gomega.NewWithT(t) per test conventions.
Suggested fix
import (
"testing"
"time"
"github.com/google/uuid"
+ "github.com/onsi/gomega"
"github.com/samber/lo"
"github.com/flanksource/duty/models"
)
func TestAddAgentConfig(t *testing.T) {
+ g := gomega.NewWithT(t)
agentID := uuid.New()
agent := models.Agent{
ID: agentID,
Name: "test-agent",
}
@@
pushData.AddAgentConfig(agent)
- if len(pushData.ConfigItems) != 1 {
- t.Fatalf("expected 1 config item, got %d", len(pushData.ConfigItems))
- }
+ g.Expect(pushData.ConfigItems).To(gomega.HaveLen(1))
ci := pushData.ConfigItems[0]
- if ci.ID != agentID {
- t.Errorf("expected agent config item ID to be %s, got %s", agentID, ci.ID)
- }
- if lo.FromPtr(ci.Name) != "test-agent" {
- t.Errorf("expected agent config item name to be 'test-agent', got %s", lo.FromPtr(ci.Name))
- }
- if lo.FromPtr(ci.ScraperID) != uuid.Nil.String() {
- t.Errorf("expected scraper_id to be nil UUID, got %s", lo.FromPtr(ci.ScraperID))
- }
+ g.Expect(ci.ID).To(gomega.Equal(agentID))
+ g.Expect(lo.FromPtr(ci.Name)).To(gomega.Equal("test-agent"))
+ g.Expect(lo.FromPtr(ci.ScraperID)).To(gomega.Equal(uuid.Nil.String()))
- if pushData.ConfigChanges[0].ConfigID != agentID.String() {
- t.Errorf("expected config change config_id to be %s, got %s", agentID, pushData.ConfigChanges[0].ConfigID)
- }
+ g.Expect(pushData.ConfigChanges).To(gomega.HaveLen(1))
+ g.Expect(pushData.ConfigChanges[0].ConfigID).To(gomega.Equal(agentID.String()))
- if len(pushData.ConfigScrapers) != 1 {
- t.Fatalf("expected 1 config scraper, got %d", len(pushData.ConfigScrapers))
- }
- if pushData.ConfigScrapers[0].ID == uuid.Nil {
- t.Error("system scraper should have been filtered out")
- }
+ g.Expect(pushData.ConfigScrapers).To(gomega.HaveLen(1))
+ g.Expect(pushData.ConfigScrapers[0].ID).ToNot(gomega.Equal(uuid.Nil))
- if len(pushData.ConfigItemsLastScrapedTime) != 1 {
- t.Fatalf("expected 1 last scraped time entry, got %d", len(pushData.ConfigItemsLastScrapedTime))
- }
- if pushData.ConfigItemsLastScrapedTime[0].ConfigID != agentID {
- t.Errorf("expected last scraped time config_id to be %s, got %s", agentID, pushData.ConfigItemsLastScrapedTime[0].ConfigID)
- }
+ g.Expect(pushData.ConfigItemsLastScrapedTime).To(gomega.HaveLen(1))
+ g.Expect(pushData.ConfigItemsLastScrapedTime[0].ConfigID).To(gomega.Equal(agentID))
}As per coding guidelines, **/*_test.go: Always use github.com/onsi/gomega package for assertions in test files; when using gomega with native go tests, use the gomega.NewWithT(t) approach for assertions.
📝 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
| func TestAddAgentConfig(t *testing.T) { | |
| agentID := uuid.New() | |
| agent := models.Agent{ | |
| ID: agentID, | |
| Name: "test-agent", | |
| } | |
| now := time.Now() | |
| pushData := &PushData{ | |
| ConfigItems: []models.ConfigItem{ | |
| { | |
| ID: uuid.Nil, | |
| Type: lo.ToPtr("MissionControl::ShouldBeFiltered"), | |
| }, | |
| { | |
| ID: uuid.New(), | |
| Type: lo.ToPtr("MissionControl::Agent"), | |
| }, | |
| }, | |
| ConfigChanges: []models.ConfigChange{ | |
| {ConfigID: uuid.Nil.String()}, | |
| }, | |
| ConfigScrapers: []models.ConfigScraper{ | |
| {ID: uuid.Nil}, | |
| {ID: uuid.New()}, | |
| }, | |
| ConfigItemsLastScrapedTime: []models.ConfigItemLastScrapedTime{ | |
| {ConfigID: uuid.Nil, LastScrapedTime: &now}, | |
| }, | |
| } | |
| pushData.AddAgentConfig(agent) | |
| // Config item with uuid.Nil ID should be filtered out | |
| if len(pushData.ConfigItems) != 1 { | |
| t.Fatalf("expected 1 config item, got %d", len(pushData.ConfigItems)) | |
| } | |
| // MissionControl::Agent item should have ID set to agent ID | |
| ci := pushData.ConfigItems[0] | |
| if ci.ID != agentID { | |
| t.Errorf("expected agent config item ID to be %s, got %s", agentID, ci.ID) | |
| } | |
| if lo.FromPtr(ci.Name) != "test-agent" { | |
| t.Errorf("expected agent config item name to be 'test-agent', got %s", lo.FromPtr(ci.Name)) | |
| } | |
| if lo.FromPtr(ci.ScraperID) != uuid.Nil.String() { | |
| t.Errorf("expected scraper_id to be nil UUID, got %s", lo.FromPtr(ci.ScraperID)) | |
| } | |
| // Config changes with uuid.Nil config_id should be remapped | |
| if pushData.ConfigChanges[0].ConfigID != agentID.String() { | |
| t.Errorf("expected config change config_id to be %s, got %s", agentID, pushData.ConfigChanges[0].ConfigID) | |
| } | |
| // System scraper should be filtered out | |
| if len(pushData.ConfigScrapers) != 1 { | |
| t.Fatalf("expected 1 config scraper, got %d", len(pushData.ConfigScrapers)) | |
| } | |
| if pushData.ConfigScrapers[0].ID == uuid.Nil { | |
| t.Error("system scraper should have been filtered out") | |
| } | |
| // Last scraped time with uuid.Nil config_id should be remapped to agent ID | |
| if len(pushData.ConfigItemsLastScrapedTime) != 1 { | |
| t.Fatalf("expected 1 last scraped time entry, got %d", len(pushData.ConfigItemsLastScrapedTime)) | |
| } | |
| if pushData.ConfigItemsLastScrapedTime[0].ConfigID != agentID { | |
| t.Errorf("expected last scraped time config_id to be %s, got %s", agentID, pushData.ConfigItemsLastScrapedTime[0].ConfigID) | |
| } | |
| } | |
| func TestAddAgentConfig(t *testing.T) { | |
| g := gomega.NewWithT(t) | |
| agentID := uuid.New() | |
| agent := models.Agent{ | |
| ID: agentID, | |
| Name: "test-agent", | |
| } | |
| now := time.Now() | |
| pushData := &PushData{ | |
| ConfigItems: []models.ConfigItem{ | |
| { | |
| ID: uuid.Nil, | |
| Type: lo.ToPtr("MissionControl::ShouldBeFiltered"), | |
| }, | |
| { | |
| ID: uuid.New(), | |
| Type: lo.ToPtr("MissionControl::Agent"), | |
| }, | |
| }, | |
| ConfigChanges: []models.ConfigChange{ | |
| {ConfigID: uuid.Nil.String()}, | |
| }, | |
| ConfigScrapers: []models.ConfigScraper{ | |
| {ID: uuid.Nil}, | |
| {ID: uuid.New()}, | |
| }, | |
| ConfigItemsLastScrapedTime: []models.ConfigItemLastScrapedTime{ | |
| {ConfigID: uuid.Nil, LastScrapedTime: &now}, | |
| }, | |
| } | |
| pushData.AddAgentConfig(agent) | |
| // Config item with uuid.Nil ID should be filtered out | |
| g.Expect(pushData.ConfigItems).To(gomega.HaveLen(1)) | |
| ci := pushData.ConfigItems[0] | |
| g.Expect(ci.ID).To(gomega.Equal(agentID)) | |
| g.Expect(lo.FromPtr(ci.Name)).To(gomega.Equal("test-agent")) | |
| g.Expect(lo.FromPtr(ci.ScraperID)).To(gomega.Equal(uuid.Nil.String())) | |
| // Config changes with uuid.Nil config_id should be remapped | |
| g.Expect(pushData.ConfigChanges).To(gomega.HaveLen(1)) | |
| g.Expect(pushData.ConfigChanges[0].ConfigID).To(gomega.Equal(agentID.String())) | |
| // System scraper should be filtered out | |
| g.Expect(pushData.ConfigScrapers).To(gomega.HaveLen(1)) | |
| g.Expect(pushData.ConfigScrapers[0].ID).ToNot(gomega.Equal(uuid.Nil)) | |
| // Last scraped time with uuid.Nil config_id should be remapped to agent ID | |
| g.Expect(pushData.ConfigItemsLastScrapedTime).To(gomega.HaveLen(1)) | |
| g.Expect(pushData.ConfigItemsLastScrapedTime[0].ConfigID).To(gomega.Equal(agentID)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@upstream/upstream_test.go` around lines 15 - 86, TestAddAgentConfig uses
native t.Errorf/t.Fatalf assertions; replace them with Gomega by creating g :=
gomega.NewWithT(t) at start of the test and convert each assertion to
g.Expect(...) style—e.g., check lengths with
g.Expect(len(pushData.ConfigItems)).To(Equal(1)), equality checks for IDs/names
with g.Expect(...).To(Equal(...)), and ensure boolean conditions use appropriate
matchers. Also add the github.com/onsi/gomega import and remove direct
t.Fatalf/t.Errorf calls, keeping the rest of the test logic (references:
TestAddAgentConfig, pushData.AddAgentConfig, pushData.ConfigItems,
pushData.ConfigChanges, pushData.ConfigScrapers,
pushData.ConfigItemsLastScrapedTime).
Comment on lines
+357
to
+362
| // Update agent's last scraped times with correct config ID | ||
| for i, ci := range t.ConfigItemsLastScrapedTime { | ||
| if ci.ConfigID == uuid.Nil { | ||
| t.ConfigItemsLastScrapedTime[i].ConfigID = agent.ID | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Guard remap to avoid dangling config_id assignments.
Line 357 remaps every nil ConfigID to agent.ID without verifying that an agent config item is present in t.ConfigItems. That can produce invalid config_items_last_scraped_time references in partial payloads.
Suggested fix
// Update agent's last scraped times with correct config ID
+ agentConfigID := uuid.Nil
+ for _, item := range t.ConfigItems {
+ if lo.FromPtr(item.Type) == "MissionControl::Agent" {
+ agentConfigID = item.ID
+ break
+ }
+ }
+
for i, ci := range t.ConfigItemsLastScrapedTime {
- if ci.ConfigID == uuid.Nil {
- t.ConfigItemsLastScrapedTime[i].ConfigID = agent.ID
+ if ci.ConfigID == uuid.Nil && agentConfigID != uuid.Nil {
+ t.ConfigItemsLastScrapedTime[i].ConfigID = agentConfigID
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@upstream/upstream.go` around lines 357 - 362, Only remap nil ConfigID entries
in t.ConfigItemsLastScrapedTime when there is actually an agent config item in
t.ConfigItems; before the loop over t.ConfigItemsLastScrapedTime, scan
t.ConfigItems for a config item matching the current agent (e.g. check for
ci.AgentID == agent.ID or whichever field links config items to the agent) and
set a boolean like hasAgentConfig; then perform the existing remap
(t.ConfigItemsLastScrapedTime[i].ConfigID = agent.ID) only if hasAgentConfig is
true, leaving entries untouched otherwise to avoid creating dangling config_id
references.
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
Bug Fixes
Tests