carverauto/serviceradar
1
1
Fork 0
Code Issues 190 Pull requests 30 Projects Releases 177 Packages Wiki Activity Actions

fixing drift wip #2485

Merged
mfreeman451 merged 7 commits from refs/pull/2485/head into main 2025-11-28 19:07:15 +00:00
Conversation 2 Commits 7 Files changed 26 +1910 -160
mfreeman451 commented 2025-11-28 01:26:51 +00:00 (Migrated from github.com)
Owner
Copy link

Imported from GitHub pull request.

Original GitHub pull request: #2028
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2028
Original created: 2025-11-28T01:26:51Z
Original updated: 2025-11-28T19:12:58Z
Original head: carverauto/serviceradar:bug/dire_device_drift
Original base: main
Original merged: 2025-11-28T19:07:15Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

  • I have signed the DCO?
  • The build completes without errors?
  • All tests are passing when running make test?

PR Type

Bug fix

Description

  • Prevent impossible promotion policies when fingerprinting disabled

  • Update all container image tags to new SHA for drift fix

  • Disable fingerprint requirement in Helm values configuration

  • Mark drift mitigation task as completed in tracking document

Diagram Walkthrough

flowchart LR
  A["Fingerprinting Disabled"] -->|"Check Config"| B["Promotion Policy Validation"]
  B -->|"Conflict Detected"| C["Auto-disable RequireFingerprint"]
  C -->|"Applied in"| D["Config & Helm Values"]
  D -->|"Result"| E["Consistent Identity Reconciliation"]

File Walkthrough
Relevant files
Bug fix
config.go
Add fingerprint policy conflict validation                             

pkg/core/config.go

  • Added validation logic to prevent impossible promotion policies
  • Automatically disables RequireFingerprint when fingerprinting is
    disabled
  • Prevents configuration conflicts that could cause device drift
 +5/-0     
Configuration changes
values.yaml
Update image tags and disable fingerprint requirement       

helm/serviceradar/values.yaml

  • Updated all container image tags from old SHA to new SHA
    (sha-13d9cc627541190980bbad253ae6b3484a2648a0)
  • Changed requireFingerprint from true to false in promotion
    configuration
  • Ensures consistent image versions across all services
 +20/-20 
Documentation
tasks.md
Mark drift mitigation task as completed                                   

openspec/changes/add-identity-reconciliation-engine/tasks.md

  • Marked task 1.16 as completed with checkbox
  • Documents drift mitigation work including fingerprint gating fix
  • References new image SHA and faker Helm value pinning
 +1/-0     
Imported from GitHub pull request. Original GitHub pull request: #2028 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2028 Original created: 2025-11-28T01:26:51Z Original updated: 2025-11-28T19:12:58Z Original head: carverauto/serviceradar:bug/dire_device_drift Original base: main Original merged: 2025-11-28T19:07:15Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Bug fix ___ ### **Description** - Prevent impossible promotion policies when fingerprinting disabled - Update all container image tags to new SHA for drift fix - Disable fingerprint requirement in Helm values configuration - Mark drift mitigation task as completed in tracking document ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Fingerprinting Disabled"] -->|"Check Config"| B["Promotion Policy Validation"] B -->|"Conflict Detected"| C["Auto-disable RequireFingerprint"] C -->|"Applied in"| D["Config & Helm Values"] D -->|"Result"| E["Consistent Identity Reconciliation"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix</strong></td><td><table> <tr> <td> <details> <summary><strong>config.go</strong><dd><code>Add fingerprint policy conflict validation</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> pkg/core/config.go <ul><li>Added validation logic to prevent impossible promotion policies<br> <li> Automatically disables <code>RequireFingerprint</code> when fingerprinting is <br>disabled<br> <li> Prevents configuration conflicts that could cause device drift</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2028/files#diff-a2dcdf3bac7ca59032d52f7a0635b5dbcfcb83a6f07cc45db8f57fba0cc33c30">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> </table></td></tr><tr><td><strong>Configuration changes</strong></td><td><table> <tr> <td> <details> <summary><strong>values.yaml</strong><dd><code>Update image tags and disable fingerprint requirement</code>&nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> helm/serviceradar/values.yaml <ul><li>Updated all container image tags from old SHA to new SHA <br>(sha-13d9cc627541190980bbad253ae6b3484a2648a0)<br> <li> Changed <code>requireFingerprint</code> from true to false in promotion <br>configuration<br> <li> Ensures consistent image versions across all services</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2028/files#diff-d4449c7cb70362554b274f81eae5a4b81a8e81df494282e383d1b7ea3871c452">+20/-20</a>&nbsp; </td> </tr> </table></td></tr><tr><td><strong>Documentation</strong></td><td><table> <tr> <td> <details> <summary><strong>tasks.md</strong><dd><code>Mark drift mitigation task as completed</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> openspec/changes/add-identity-reconciliation-engine/tasks.md <ul><li>Marked task 1.16 as completed with checkbox<br> <li> Documents drift mitigation work including fingerprint gating fix<br> <li> References new image SHA and faker Helm value pinning</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2028/files#diff-7d7c4d6b4718fd5255ec47aa62dee71b5da2c83ff2160164603454379b618181">+1/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> </table></td></tr></tr></tbody></table> </details> ___
qodo-code-review[bot] commented 2025-11-28 01:27:19 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2028#issuecomment-3587645898
Original created: 2025-11-28T01:27:19Z

You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
⚪
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

⚪
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing auditing: New configuration mutation disabling cfg.Promotion.RequireFingerprint occurs without any
audit/log entry recording who/what triggered the change and its outcome.

Referred Code 
// Avoid impossible promotion policies: fingerprint cannot be required when fingerprinting is disabled.
if !cfg.Fingerprinting.Enabled && cfg.Promotion.RequireFingerprint {
	cfg.Promotion.RequireFingerprint = false
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Silent auto-fix: The code auto-disables RequireFingerprint when fingerprinting is disabled without
surfacing a warning or error, which may hide configuration mistakes and reduce
observability.

Referred Code 
// Avoid impossible promotion policies: fingerprint cannot be required when fingerprinting is disabled.
if !cfg.Fingerprinting.Enabled && cfg.Promotion.RequireFingerprint {
	cfg.Promotion.RequireFingerprint = false
}

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2028#issuecomment-3587645898 Original created: 2025-11-28T01:27:19Z --- _You are nearing your monthly Qodo Merge usage quota. For more information, please visit [here](https://qodo-merge-docs.qodo.ai/installation/qodo_merge/#cloud-users)._ ## PR Compliance Guide 🔍 <!-- https://github.com/carverauto/serviceradar/commit/f29d4f40c12c4a560dfa5703d451352829637a1f --> Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td>🟢</td><td><details><summary><strong>No security concerns identified</strong></summary> No security vulnerabilities detected by AI analysis. Human verification advised for critical code. </details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary>🎫 <strong>No ticket provided </strong></summary> - [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true --> </details></td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=4>🟢</td><td> <details><summary><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=2>⚪</td> <td><details> <summary><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2028/files#diff-a2dcdf3bac7ca59032d52f7a0635b5dbcfcb83a6f07cc45db8f57fba0cc33c30R284-R287'><strong>Missing auditing</strong></a>: New configuration mutation disabling <code>cfg.Promotion.RequireFingerprint</code> occurs without any <br>audit/log entry recording who/what triggered the change and its outcome.<br> <details open><summary>Referred Code</summary> ```go // Avoid impossible promotion policies: fingerprint cannot be required when fingerprinting is disabled. if !cfg.Fingerprinting.Enabled && cfg.Promotion.RequireFingerprint { cfg.Promotion.RequireFingerprint = false } ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2028/files#diff-a2dcdf3bac7ca59032d52f7a0635b5dbcfcb83a6f07cc45db8f57fba0cc33c30R284-R287'><strong>Silent auto-fix</strong></a>: The code auto-disables <code>RequireFingerprint</code> when fingerprinting is disabled without <br>surfacing a warning or error, which may hide configuration mistakes and reduce <br>observability.<br> <details open><summary>Referred Code</summary> ```go // Avoid impossible promotion policies: fingerprint cannot be required when fingerprinting is disabled. if !cfg.Fingerprinting.Enabled && cfg.Promotion.RequireFingerprint { cfg.Promotion.RequireFingerprint = false } ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2"> - [ ] Update <!-- /compliance --update_compliance=true --> </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>
qodo-code-review[bot] commented 2025-11-28 01:28:16 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2028#issuecomment-3587646851
Original created: 2025-11-28T01:28:16Z

You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Decouple service image updates from fixes

The PR combines a logic fix in the core service with a mass update of all
container image tags. It is suggested to decouple these changes by only updating
the image for the affected service to reduce deployment risk.

Examples:

helm/serviceradar/values.yaml [5-27] 
    # ServiceRadar UUID identity system - generates stable device IDs based on strong identifiers
    core: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
    web: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
    nats: "2.12.2-alpine"
    datasvc: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
    agent: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
    poller: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
    snmpChecker: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
    dbEventWriter: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
    otel: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"

 ... (clipped 13 lines)
pkg/core/config.go [284-287] 
	// Avoid impossible promotion policies: fingerprint cannot be required when fingerprinting is disabled.
	if !cfg.Fingerprinting.Enabled && cfg.Promotion.RequireFingerprint {
		cfg.Promotion.RequireFingerprint = false
	}

Solution Walkthrough:

Before:

# PR bundles a logic fix with updates to all service images

# helm/serviceradar/values.yaml
image:
  tags:
    core: "new-sha"
    web: "new-sha"
    datasvc: "new-sha"
    agent: "new-sha"
    # ... and all other services updated

# pkg/core/config.go
func applyIdentityDefaults(cfg *Config) {
  // ...
  if !cfg.Fingerprinting.Enabled && cfg.Promotion.RequireFingerprint {
    cfg.Promotion.RequireFingerprint = false
  }
}

After:

# PR only contains the logic fix and the image update for the affected service

# helm/serviceradar/values.yaml
image:
  tags:
    core: "new-sha" # Only core service is updated
    web: "old-sha"
    datasvc: "old-sha"
    agent: "old-sha"
    # ... other services remain on old SHA

# pkg/core/config.go
func applyIdentityDefaults(cfg *Config) {
  // ...
  if !cfg.Fingerprinting.Enabled && cfg.Promotion.RequireFingerprint {
    cfg.Promotion.RequireFingerprint = false
  }
}
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a risky practice of bundling a specific service fix with a fleet-wide image update, which increases deployment complexity and risk.

Medium
General
✅ Use YAML anchors for image tags
Suggestion Impact:The commit added an appTag anchor and replaced repeated image tags with references (*appTag) for most services, reducing duplication.

code diff:

+    appTag: &appTag "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
     # ServiceRadar UUID identity system - generates stable device IDs based on strong identifiers
-    core: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-    web: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
+    core: *appTag
+    web: *appTag
     nats: "2.12.2-alpine"
-    datasvc: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-    agent: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-    poller: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-    snmpChecker: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-    dbEventWriter: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-    otel: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-    mapper: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-    trapd: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-    flowgger: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-    zen: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
+    datasvc: *appTag
+    agent: *appTag
+    poller: *appTag
+    snmpChecker: *appTag
+    dbEventWriter: *appTag
+    otel: *appTag
+    mapper: *appTag
+    trapd: *appTag
+    flowgger: *appTag
+    zen: *appTag
     # Armis sync uses empty DeviceID - registry generates ServiceRadar UUIDs
-    sync: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-    rperfClient: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-    faker: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-    rperfChecker: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-    tools: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-    srql: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
+    sync: *appTag
+    rperfClient: *appTag
+    faker: "sha-f29d4f40c12c4a560dfa5703d451352829637a1f"
+    rperfChecker: *appTag
+    tools: *appTag
+    srql: *appTag
     kong: "3.7"
-    kongConfig: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
+    kongConfig: *appTag

Use YAML anchors in values.yaml to define the common image tag once and reuse it
across multiple services, improving maintainability.

helm/serviceradar/values.yaml [4-27]

 tags:
+  appTag: &appTag "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
   # ServiceRadar UUID identity system - generates stable device IDs based on strong identifiers
-  core: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-  web: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
+  core: *appTag
+  web: *appTag
   nats: "2.12.2-alpine"
-  datasvc: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-  agent: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-  poller: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-  snmpChecker: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-  dbEventWriter: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-  otel: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-  mapper: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-  trapd: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-  flowgger: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-  zen: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
+  datasvc: *appTag
+  agent: *appTag
+  poller: *appTag
+  snmpChecker: *appTag
+  dbEventWriter: *appTag
+  otel: *appTag
+  mapper: *appTag
+  trapd: *appTag
+  flowgger: *appTag
+  zen: *appTag
   # Armis sync uses empty DeviceID - registry generates ServiceRadar UUIDs
-  sync: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-  rperfClient: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-  faker: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-  rperfChecker: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-  tools: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
-  srql: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
+  sync: *appTag
+  rperfClient: *appTag
+  faker: *appTag
+  rperfChecker: *appTag
+  tools: *appTag
+  srql: *appTag
   kong: "3.7"
-  kongConfig: "sha-13d9cc627541190980bbad253ae6b3484a2648a0"
+  kongConfig: *appTag

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion improves the maintainability of the Helm values.yaml file by using YAML anchors to avoid repeating the same image tag, reducing the risk of human error during updates.

Medium
✅ Log when configuration is silently changed
Suggestion Impact:A warning log line was added before setting RequireFingerprint to false when fingerprinting is disabled, matching the suggestion.

code diff:

 	// Avoid impossible promotion policies: fingerprint cannot be required when fingerprinting is disabled.
 	if !cfg.Fingerprinting.Enabled && cfg.Promotion.RequireFingerprint {
+		log.Println("Warning: Promotion.RequireFingerprint is true, but Fingerprinting.Enabled is false. Forcing RequireFingerprint to false to prevent promotion stalls.")
 		cfg.Promotion.RequireFingerprint = false

Add a warning log when RequireFingerprint is automatically set to false to
notify operators of the configuration override.

pkg/core/config.go [284-287]

 // Avoid impossible promotion policies: fingerprint cannot be required when fingerprinting is disabled.
 if !cfg.Fingerprinting.Enabled && cfg.Promotion.RequireFingerprint {
+	log.Println("Warning: Promotion.RequireFingerprint is true, but Fingerprinting.Enabled is false. Forcing RequireFingerprint to false to prevent promotion stalls.")
 	cfg.Promotion.RequireFingerprint = false
 }

[Suggestion processed]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that silently changing a configuration can be confusing and improves observability by adding a log message, which is a good practice for maintainability.

Low
  • Update
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2028#issuecomment-3587646851 Original created: 2025-11-28T01:28:16Z --- _You are nearing your monthly Qodo Merge usage quota. For more information, please visit [here](https://qodo-merge-docs.qodo.ai/installation/qodo_merge/#cloud-users)._ ## PR Code Suggestions ✨ <!-- f29d4f4 --> Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>High-level</td> <td> <details><summary>Decouple service image updates from fixes</summary> ___ **The PR combines a logic fix in the <code>core</code> service with a mass update of all <br>container image tags. It is suggested to decouple these changes by only updating <br>the image for the affected service to reduce deployment risk.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2028/files#diff-d4449c7cb70362554b274f81eae5a4b81a8e81df494282e383d1b7ea3871c452R5-R27">helm/serviceradar/values.yaml [5-27]</a> </summary> ```yaml # ServiceRadar UUID identity system - generates stable device IDs based on strong identifiers core: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" web: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" nats: "2.12.2-alpine" datasvc: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" agent: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" poller: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" snmpChecker: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" dbEventWriter: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" otel: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" ... (clipped 13 lines) ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2028/files#diff-a2dcdf3bac7ca59032d52f7a0635b5dbcfcb83a6f07cc45db8f57fba0cc33c30R284-R287">pkg/core/config.go [284-287]</a> </summary> ```go // Avoid impossible promotion policies: fingerprint cannot be required when fingerprinting is disabled. if !cfg.Fingerprinting.Enabled && cfg.Promotion.RequireFingerprint { cfg.Promotion.RequireFingerprint = false } ``` </details> ### Solution Walkthrough: #### Before: ```go # PR bundles a logic fix with updates to all service images # helm/serviceradar/values.yaml image: tags: core: "new-sha" web: "new-sha" datasvc: "new-sha" agent: "new-sha" # ... and all other services updated # pkg/core/config.go func applyIdentityDefaults(cfg *Config) { // ... if !cfg.Fingerprinting.Enabled && cfg.Promotion.RequireFingerprint { cfg.Promotion.RequireFingerprint = false } } ``` #### After: ```go # PR only contains the logic fix and the image update for the affected service # helm/serviceradar/values.yaml image: tags: core: "new-sha" # Only core service is updated web: "old-sha" datasvc: "old-sha" agent: "old-sha" # ... other services remain on old SHA # pkg/core/config.go func applyIdentityDefaults(cfg *Config) { // ... if !cfg.Fingerprinting.Enabled && cfg.Promotion.RequireFingerprint { cfg.Promotion.RequireFingerprint = false } } ``` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a risky practice of bundling a specific service fix with a fleet-wide image update, which increases deployment complexity and risk. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=2>General</td> <td> <details><summary>✅ <s>Use YAML anchors for image tags</s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>The commit added an appTag anchor and replaced repeated image tags with references (*appTag) for most services, reducing duplication. code diff: ```diff + appTag: &appTag "sha-13d9cc627541190980bbad253ae6b3484a2648a0" # ServiceRadar UUID identity system - generates stable device IDs based on strong identifiers - core: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - web: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" + core: *appTag + web: *appTag nats: "2.12.2-alpine" - datasvc: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - agent: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - poller: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - snmpChecker: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - dbEventWriter: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - otel: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - mapper: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - trapd: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - flowgger: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - zen: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" + datasvc: *appTag + agent: *appTag + poller: *appTag + snmpChecker: *appTag + dbEventWriter: *appTag + otel: *appTag + mapper: *appTag + trapd: *appTag + flowgger: *appTag + zen: *appTag # Armis sync uses empty DeviceID - registry generates ServiceRadar UUIDs - sync: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - rperfClient: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - faker: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - rperfChecker: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - tools: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - srql: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" + sync: *appTag + rperfClient: *appTag + faker: "sha-f29d4f40c12c4a560dfa5703d451352829637a1f" + rperfChecker: *appTag + tools: *appTag + srql: *appTag kong: "3.7" - kongConfig: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" + kongConfig: *appTag ``` </details> ___ **Use YAML anchors in <code>values.yaml</code> to define the common image tag once and reuse it <br>across multiple services, improving maintainability.** [helm/serviceradar/values.yaml [4-27]](https://github.com/carverauto/serviceradar/pull/2028/files#diff-d4449c7cb70362554b274f81eae5a4b81a8e81df494282e383d1b7ea3871c452R4-R27) ```diff tags: + appTag: &appTag "sha-13d9cc627541190980bbad253ae6b3484a2648a0" # ServiceRadar UUID identity system - generates stable device IDs based on strong identifiers - core: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - web: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" + core: *appTag + web: *appTag nats: "2.12.2-alpine" - datasvc: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - agent: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - poller: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - snmpChecker: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - dbEventWriter: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - otel: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - mapper: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - trapd: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - flowgger: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - zen: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" + datasvc: *appTag + agent: *appTag + poller: *appTag + snmpChecker: *appTag + dbEventWriter: *appTag + otel: *appTag + mapper: *appTag + trapd: *appTag + flowgger: *appTag + zen: *appTag # Armis sync uses empty DeviceID - registry generates ServiceRadar UUIDs - sync: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - rperfClient: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - faker: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - rperfChecker: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - tools: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" - srql: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" + sync: *appTag + rperfClient: *appTag + faker: *appTag + rperfChecker: *appTag + tools: *appTag + srql: *appTag kong: "3.7" - kongConfig: "sha-13d9cc627541190980bbad253ae6b3484a2648a0" + kongConfig: *appTag ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion improves the maintainability of the Helm `values.yaml` file by using YAML anchors to avoid repeating the same image tag, reducing the risk of human error during updates. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>✅ <s>Log when configuration is silently changed</s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>A warning log line was added before setting RequireFingerprint to false when fingerprinting is disabled, matching the suggestion. code diff: ```diff // Avoid impossible promotion policies: fingerprint cannot be required when fingerprinting is disabled. if !cfg.Fingerprinting.Enabled && cfg.Promotion.RequireFingerprint { + log.Println("Warning: Promotion.RequireFingerprint is true, but Fingerprinting.Enabled is false. Forcing RequireFingerprint to false to prevent promotion stalls.") cfg.Promotion.RequireFingerprint = false ``` </details> ___ **Add a warning log when <code>RequireFingerprint</code> is automatically set to <code>false</code> to <br>notify operators of the configuration override.** [pkg/core/config.go [284-287]](https://github.com/carverauto/serviceradar/pull/2028/files#diff-a2dcdf3bac7ca59032d52f7a0635b5dbcfcb83a6f07cc45db8f57fba0cc33c30R284-R287) ```diff // Avoid impossible promotion policies: fingerprint cannot be required when fingerprinting is disabled. if !cfg.Fingerprinting.Enabled && cfg.Promotion.RequireFingerprint { + log.Println("Warning: Promotion.RequireFingerprint is true, but Fingerprinting.Enabled is false. Forcing RequireFingerprint to false to prevent promotion stalls.") cfg.Promotion.RequireFingerprint = false } ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies that silently changing a configuration can be confusing and improves observability by adding a log message, which is a good practice for maintainability. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update <!-- /improve_multi --more_suggestions=true --> </td><td></td></tr></tbody></table>
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
1week

   
2weeks

   
Failed compliance check

   
IP cameras

   
NATS

NATS JetStream

  
Possible security concern

   
Review effort 1/5

   
Review effort 2/5

   
Review effort 3/5

   
Review effort 4/5

   
Review effort 5/5

   
UI

   
aardvark

   
accessibility

   
amd64

   
api

   
arm64

   
auth

   
back-end

   
bgp

   
blog

   
bug

Something isn't working

  
build

   
checkers

   
ci-cd

continuous integration-continuous deployments

  
cleanup

   
cnpg

cloud-native postgres

  
codex

   
core

core service

  
dependencies

Pull requests that update a dependency file

  
device-management

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
dusk

   
ebpf

   
enhancement

New feature or request

  
eta 1d

   
eta 1hr

   
eta 3d

   
eta 3hr

   
feature

   
fieldsurvey

   
github_actions

Pull requests that update GitHub Actions code

  
go

Pull requests that update Go code

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
javascript

Pull requests that update Javascript code

  
k8s

   
log-collector

   
mapper

   
mtr

multi traceroute

  
needs-triage

   
netflow

   
network-sweep

   
observability

   
oracle

Oracle Linux related issues

  
otel

opentelemetry logs, traces, metrics

  
plug-in

   
proton

timeplus proton streaming database

  
python

   
question

Further information is requested

  
reddit

   
redhat

   
research

   
rperf

   
rperf-checker

   
rust

Pull requests that update rust code

  
sdk

   
security

   
serviceradar-agent

   
serviceradar-agent-gateway

   
serviceradar-web

   
serviceradar-web-ng

   
siem

   
snmp

   
sysmon

   
topology

   
ubiquiti

   
wasm

   
wontfix

This will not be worked on

  
zen-engine
No labels
1week
2weeks
Failed compliance check
IP cameras
NATS
Possible security concern
Review effort 1/5
Review effort 2/5
Review effort 3/5
Review effort 4/5
Review effort 5/5
UI
aardvark
accessibility
amd64
api
arm64
auth
back-end
bgp
blog
bug
build
checkers
ci-cd
cleanup
cnpg
codex
core
dependencies
device-management
documentation
duplicate
dusk
ebpf
enhancement
eta 1d
eta 1hr
eta 3d
eta 3hr
feature
fieldsurvey
github_actions
go
good first issue
help wanted
invalid
javascript
k8s
log-collector
mapper
mtr
needs-triage
netflow
network-sweep
observability
oracle
otel
plug-in
proton
python
question
reddit
redhat
research
rperf
rperf-checker
rust
sdk
security
serviceradar-agent
serviceradar-agent-gateway
serviceradar-web
serviceradar-web-ng
siem
snmp
sysmon
topology
ubiquiti
wasm
wontfix
zen-engine
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
carverauto/serviceradar!2485
No description provided.