fix: Both path and paths need to be supported to address the breaking change until the next release #290

presatish · 2023-09-15T06:42:17Z

If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/edgex-go/blob/main/.github/Contributing.md

PR Checklist

Please check if your PR fulfills the following requirements:

I am not introducing a breaking change (if you are, flag in conventional commit message with BREAKING CHANGE: describing the break)
I am not introducing a new dependency (add notes below if you are)
I have added unit tests for the new feature or bug fix (if not, why?)
I have fully tested (add details below) this the new feature or bug fix (if not, why?)
I have opened a PR for the related docs change (if not, why?)

Testing Instructions

make test
make build
make docker
Get the edgex core and device usb service running in 3.0 version (Minnesota)
The usb devices get registered in edgex with Path as one of the protocol properties which is a string.
Stop the containers without deleting the volume (make down)
Checkout the main version of edgex compose and get the core services running.
Get this version of device service running natively or even in docker
The same devices get discovered again and Path changes to Paths which is an array of string.

New Dependency Instructions (If applicable)

Signed-off-by: preethi-satishcandra <[email protected]>

codecov-commenter · 2023-09-15T06:54:43Z

Codecov Report

Merging #290 (18a7ff3) into main (608e40c) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff            @@
##            main    #290      +/-   ##
========================================
- Coverage   4.79%   4.74%   -0.05%     
========================================
  Files          8       8              
  Lines       1293    1306      +13     
========================================
  Hits          62      62              
- Misses      1231    1244      +13

Files Changed	Coverage Δ
internal/driver/device.go	`2.83% <ø> (ø)`
internal/driver/driver.go	`3.21% <0.00%> (-0.06%)`	⬇️
internal/driver/helper.go	`80.95% <0.00%> (ø)`

Signed-off-by: preethi-satishcandra <[email protected]>

ajcasagrande · 2023-09-18T22:49:50Z

internal/driver/helper.go


 type EdgeXErrorWrapper struct{}

 func (e EdgeXErrorWrapper) CommandError(command string, err error) errors.EdgeX {
 	return errors.NewCommonEdgeX(errors.KindServerError, fmt.Sprintf("failed to execute %s command", command), err)
 }

-// redact removes all instances of basic auth (ie. rtsp://username:password@server) from a url
+// redact removes all instances of basic auth (i.e. rtsp://username:password@server) from an url


a url is the correct grammer here: https://www.techtarget.com/whatis/feature/Which-is-correct-a-URL-or-an-URL

Even the silly IDE suggested to me that it should be an :-)

ajcasagrande · 2023-09-18T22:50:49Z

internal/driver/helper.go

@@ -14,23 +14,17 @@ import (
 	"github.com/edgexfoundry/go-mod-core-contracts/v3/errors"
 )

-const (
-	redactedStr = "//<redacted>@"


I feel this makes more sense being defined here, closer to its usage

Ok, I thought it would be better to keep all constants in one place.

ajcasagrande · 2023-09-18T22:51:33Z

internal/driver/types.go

-const (
-	RTSPServerModeInternal RTSPServerMode = "internal"
-	RTSPServerModeExternal RTSPServerMode = "external"
-	RTSPServerModeNone     RTSPServerMode = "none"
-)


these belong to the type RTSPServerMode string above, and implement an enum.

ajcasagrande · 2023-09-18T22:53:04Z

internal/driver/driver.go

 // RefreshDevicePaths checks whether the existing device matches the connected device.
 // If there is a mismatch between them, scan all paths to find the matching device
 // and update the existing device with the correct path.
 func (d *Driver) RefreshDevicePaths(cd models.Device) {
+	d.updatePathToPaths(cd)


Is this the only place that needs to call this?

yes this function is called when the service starts up and also when the discovery is called.

ajcasagrande · 2023-09-18T22:54:42Z

internal/driver/driver.go

+	if value, ok := cd.Protocols[UsbProtocol][Path]; ok {
+		if value != nil {
+			cd.Protocols[UsbProtocol][Paths] = []string{value.(string)}
+			delete(cd.Protocols[UsbProtocol], "Path")


I thought we had discussed keeping both fields in the metadata because otherwise, when you get a device it will be missing the Path field that used to be there. @lenny-intel thoughts?

I don't remember discussing about this, you mean creating Metadata section within protocols like how we have in onvif service? Existing Path field will be changed to Paths with this code which is easier to maintain. We have added in the docs too that Path field is deprecated and users should be using Paths. I think @lenny-intel was fine with this.

I mean keeping bath Path and Paths in the Onvif protocol properties, and using code to sync them. As long as @lenny-intel is OK with this approach I am fine as well, since this will be cleaner.

Yes, we have to keep both. My thought is if a device is loaded that has Path set, then set Paths to the single Path. That way the driver code always deals with Paths.

Signed-off-by: preethi-satishcandra <[email protected]>

ajcasagrande · 2023-09-18T23:45:42Z

internal/driver/types.go

+	PixFmtDepthZ16:     Depth,
+}
+
+const (


I would put these back where they were, or move the type definition down here. I like them being together.

Signed-off-by: preethi-satishcandra <[email protected]>

ajcasagrande

LGTM

vyshali-chitikeshi · 2023-09-27T18:45:46Z

Though its already merged into main, I did validaiton to ensure end-to-end testing (discovering usb cameras, adding them egex metadata and rest-api's) works correctly both with 'path' and 'paths' fields and it looks good.
Validated with below combinations of code:

Latest (main branch) egex and Latest (main branch) USB code - works with 'paths'
Latest (main branch) egex and 3.0 USB code - works with 'path'
3.0 egex and Latest (main branch) USB code - works with 'paths'
3.0 egex and 3.0 USB code - works with 'path

attaced scrresnhots for refecne.
latest_edgex_and_3.0_usb_device_usb.txt
3.0_edgex_and_latest_usb_device_path.txt
3.0_edgex_and_3.0_usb_device_path.txt
latest_edgex_and_latest_usb_device_path.txt

presatish added 5 commits September 11, 2023 09:25

fix: intial commit

c93d5c8

Signed-off-by: preethi-satishcandra <[email protected]>

Merge branch 'main' into support-path

f3505bc

Merge branch 'main' into support-path

388201c

fix: code to address the breaking change added

ab2285c

Signed-off-by: preethi-satishcandra <[email protected]>

fix: removed unused code

15bf18a

Signed-off-by: preethi-satishcandra <[email protected]>

presatish requested review from ajcasagrande, lenny-goodell, seanohair22 and vyshali-chitikeshi September 15, 2023 06:42

presatish linked an issue Sep 15, 2023 that may be closed by this pull request

Config variable Path changed to Paths to support real sense cameras #278

Closed

fix: cleaned up some old code

18a7ff3

Signed-off-by: preethi-satishcandra <[email protected]>

ajcasagrande requested changes Sep 18, 2023

View reviewed changes

fix: addressed code review comments

3679936

Signed-off-by: preethi-satishcandra <[email protected]>

ajcasagrande requested changes Sep 18, 2023

View reviewed changes

fix: addressed more code review comments

6bb0ca8

Signed-off-by: preethi-satishcandra <[email protected]>

ajcasagrande approved these changes Sep 19, 2023

View reviewed changes

presatish merged commit 884b56a into edgexfoundry:main Sep 19, 2023

presatish deleted the support-path branch September 19, 2023 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Both path and paths need to be supported to address the breaking change until the next release #290

fix: Both path and paths need to be supported to address the breaking change until the next release #290

presatish commented Sep 15, 2023

codecov-commenter commented Sep 15, 2023 •

edited

Loading

ajcasagrande Sep 18, 2023

presatish Sep 18, 2023

ajcasagrande Sep 18, 2023

presatish Sep 18, 2023

ajcasagrande Sep 18, 2023

ajcasagrande Sep 18, 2023

presatish Sep 18, 2023

ajcasagrande Sep 18, 2023

presatish Sep 18, 2023

ajcasagrande Sep 18, 2023

lenny-goodell Sep 18, 2023

ajcasagrande Sep 18, 2023

ajcasagrande left a comment

vyshali-chitikeshi commented Sep 27, 2023

fix: Both path and paths need to be supported to address the breaking change until the next release #290

fix: Both path and paths need to be supported to address the breaking change until the next release #290

Conversation

presatish commented Sep 15, 2023

PR Checklist

Testing Instructions

New Dependency Instructions (If applicable)

codecov-commenter commented Sep 15, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajcasagrande left a comment

Choose a reason for hiding this comment

vyshali-chitikeshi commented Sep 27, 2023

codecov-commenter commented Sep 15, 2023 •

edited

Loading