Skip to content

feat: add discovery timestamps to deviceInfo#1136

Open
sinchubhat wants to merge 1 commit into
mainfrom
issue1394-rpc-go
Open

feat: add discovery timestamps to deviceInfo#1136
sinchubhat wants to merge 1 commit into
mainfrom
issue1394-rpc-go

Conversation

@sinchubhat

@sinchubhat sinchubhat commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Add firstDiscovered (immutable) and rename lastUpdated to lastSynced.

Related to device-management-toolkit/rpc-go#1394

Migration matrix:

rpc-go new --> device-management-toolkit/rpc-go#1441
rpc-go old --> latest rpc-go main branch
console new --> #1136
console old --> latest console main branch

rpc-go console Result
new old lastSynced/firstDiscovered not understood by old console; lastUpdated no longer sent by new rpc -> timestamp not updated (absent on a fresh record; stale value retained on an existing one)
old new legacy lastUpdated mapped -> lastSynced via compat UnmarshalJSON
new new full behavior - lastSynced on every sync; firstDiscovered on first discovery
old old legacy lastUpdated (unchanged)

Note:
Deploy order: console#1136 must get merged before or with rpc-go#1441 - new rpc against old console loses the timestamps.
firstDiscovered is set once at device creation; updating an existing record won't add it. Delete the record (full deactivation) then re-sync to see it set fresh.

@codecov

codecov Bot commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 72.72727% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.68%. Comparing base (4dff529) to head (631cb5d).

Files with missing lines Patch % Lines
internal/controller/openapi/devices.go 0.00% 4 Missing ⚠️
internal/usecase/devices/usecase.go 83.33% 2 Missing and 1 partial ⚠️
internal/entity/dto/v1/device.go 81.81% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1136      +/-   ##
==========================================
+ Coverage   43.61%   43.68%   +0.07%     
==========================================
  Files         143      144       +1     
  Lines       13626    13654      +28     
==========================================
+ Hits         5943     5965      +22     
- Misses       7118     7122       +4     
- Partials      565      567       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds discovery timestamp tracking to the v1 DeviceInfo model by introducing an immutable firstDiscovered field and renaming the sync timestamp field from lastUpdated to lastSynced, updating merge behavior and tests to reflect the new semantics.

Changes:

  • Add firstDiscovered and lastSynced to dto/v1.DeviceInfo, including JSON compatibility mapping from legacy lastUpdated on input.
  • Update partial-merge behavior to treat discovered/firstDiscovered as write-once fields (when merged via nested field maps).
  • Refresh unit/integration artifacts (tests, OpenAPI examples, Postman collection) to use the new fields.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
internal/usecase/devices/usecase.go Adds write-once setters for discovered and firstDiscovered, and maps legacy lastupdated field selection to LastSynced.
internal/usecase/devices/repo_test.go Extends partial-update tests and adds a test asserting immutability of discovered/firstDiscovered during PATCH merges.
internal/entity/dto/v1/device.go Introduces FirstDiscovered and renames LastUpdatedLastSynced, with a custom UnmarshalJSON to accept legacy lastUpdated.
internal/entity/dto/v1/device_test.go Updates round-trip JSON tests and adds coverage for legacy lastUpdated input mapping to LastSynced.
internal/controller/openapi/devices.go Updates example payloads to include firstDiscovered and lastSynced.
internal/controller/httpapi/v1/devices_test.go Updates v1 HTTP tests to send/expect the new JSON fields.
integration-test/collections/console_mps_apis.postman_collection.json Updates Postman request examples to use firstDiscovered and lastSynced.

Comment thread internal/entity/dto/v1/device.go
Comment thread internal/usecase/devices/usecase.go
@sinchubhat
sinchubhat force-pushed the issue1394-rpc-go branch 2 times, most recently from 89d4360 to 7e08834 Compare July 15, 2026 04:03
Comment thread internal/entity/dto/v1/device.go
@sinchubhat

Copy link
Copy Markdown
Contributor Author

@MadhaviLosetty @sudhir-intc

Is deviceInfo.lastUpdated consumed anywhere? If not, we rename it to lastSynced and drop the compat mirror (Option 3).

The naming options:

Option 1 - Keep the name lastUpdated (no rename)
deviceInfo.lastUpdated stays as-is; we only add firstDiscovered.

  • No breaking change, no extra code.
  • Confusing next to the new top-level lastUpdate: the response will have lastUpdate (top-level, any change) and lastUpdated (deviceInfo, sync-only) - two nearly identical names, one letter apart, meaning different things.

Option 2 - Rename to lastSynced, keep a compatibility mirror (this is the copilot review comment)
Rename to deviceInfo.lastSynced, but MarshalJSON still also emits deviceInfo.lastUpdated (same value) for old clients.

  • Backward compatible AND semantically clear name (lastSynced).
  • Pointless as a rename - the wire still shows deviceInfo.lastUpdated, so we didn't actually get rid of the confusing name; we just added a second one next to it. Also stores/returns a redundant field.

Option 3 — Rename to lastSynced, drop the mirror (clean) - current code
Rename to deviceInfo.lastSynced and stop emitting deviceInfo.lastUpdated entirely.

  • Clean and unambiguous: firstDiscovered + lastSynced in deviceInfo, lastUpdate at top level. No duplicate, no near-duplicate names.
  • Technically a field rename on /api/v1. Safe only if nothing currently reads deviceInfo.lastUpdated.

Note on the names: top-level is lastUpdate, deviceInfo's old field is lastUpdated — they differ by one letter, so it's not a hard key-collision, but that near-identical pairing is exactly why a distinct name like lastSynced is worth it.

@sinchubhat
sinchubhat force-pushed the issue1394-rpc-go branch 2 times, most recently from 5ad4331 to 5c0adb7 Compare July 15, 2026 05:38
@sinchubhat
sinchubhat marked this pull request as ready for review July 16, 2026 04:27
@sinchubhat
sinchubhat requested a review from a team as a code owner July 16, 2026 04:27
@sinchubhat

Copy link
Copy Markdown
Contributor Author

Hi @sudhir-intc , checked with Madhavi/Ganesh offline on this, Going ahead with option3:
No issue with renaming lastUpdated to lastSynced. That said, we should update the following repos as well to keep things consistent:
Console – It currently consumes, persists, and returns lastUpdated in API responses.
sample-web-ui – It only declares lastUpdated in the DeviceInfo interface and doesn’t appear to use or display it anywhere.
This will help keep the naming consistent across all repos.
we can make that breaking change on console as the feature is not complete yet. Upgrading Console binary should not break, that is the most important thing to consider.

does it break the migration flow if we make this change? if not, then we are good to make this change.
No, it does not break migration. Since it is device-info column where the entire json data stores as text. From these changes, lastUpdated will be dropped. That's it.

Add firstDiscovered (immutable) and rename lastUpdated to lastSynced.

Related to device-management-toolkit/rpc-go#1394

@sudhir-intc sudhir-intc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM .. there's one minor comment please take in the suggestion.

Comment thread internal/entity/dto/v1/device.go
Comment thread internal/entity/dto/v1/device.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add discovery timestamps to device sync payloads

3 participants