Skip to content

fix: panic when device is missing CardName or SerialNumber#261

Merged
presatish merged 3 commits intoedgexfoundry:mainfrom
EdgeX-Camera-Management:fix-discover-panic
Jun 23, 2023
Merged

fix: panic when device is missing CardName or SerialNumber#261
presatish merged 3 commits intoedgexfoundry:mainfrom
EdgeX-Camera-Management:fix-discover-panic

Conversation

@ajcasagrande
Copy link
Copy Markdown
Contributor

@ajcasagrande ajcasagrande commented Jun 16, 2023 •
edited
Loading

See: https://github.com/orgs/edgexfoundry/discussions/129

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)
  • [N/A] 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

New Dependency Instructions (If applicable)

@ajcasagrande
fix: panic when device is missing CardName or SerialNumber
ff2f74a 
Signed-off-by: Anthony Casagrande <anthony.j.casagrande@intel.com>
@lenny-goodell
Copy link
Copy Markdown
Member

lenny-goodell commented Jun 16, 2023 •
edited
Loading

@ajcasagrande , please fix on main (not minnesota since there is a work around) and create a BUG issue for it and list it here: https://wiki.edgexfoundry.org/display/FA/Minnesota#Minnesota-KnownBugs

presatish
presatish reviewed Jun 16, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@presatish presatish left a comment

Choose a reason for hiding this comment

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

It would be helpful if examples under https://github.com/EdgeX-Camera-Management/device-usb-camera/blob/fix-discover-panic/cmd/res/devices are updated too. There is no CardName here.

@ajcasagrande ajcasagrande changed the base branch from minnesota to main June 20, 2023 17:53
lenny-goodell
lenny-goodell requested changes Jun 20, 2023
View reviewed changes
lenny-goodell
lenny-goodell approved these changes Jun 20, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@lenny-goodell lenny-goodell left a comment

Choose a reason for hiding this comment

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

LGTM

lenny-goodell
lenny-goodell requested changes Jun 20, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@lenny-goodell lenny-goodell left a comment

Choose a reason for hiding this comment

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

It would be helpful if examples under https://github.com/EdgeX-Camera-Management/device-usb-camera/blob/fix-discover-panic/cmd/res/devices are updated too. There is no CardName here.

Yes, and not just in the Example App, also here in this repo.
https://github.com/edgexfoundry/device-usb-camera/blob/main/cmd/res/devices/general.usb.camera.yaml.example
https://github.com/edgexfoundry/device-usb-camera/blob/main/cmd/res/devices/hp.w200.yaml.example

@ajcasagrande , please fix these as part of this PR. THX!

@ajcasagrande
fix: panic in newDevice when card name or serial is missing
7f81971 
Signed-off-by: Anthony Casagrande <anthony.j.casagrande@intel.com>
lenny-goodell
lenny-goodell requested changes Jun 21, 2023
View reviewed changes
@ajcasagrande
fix: add device name to PatchDevice call
a8a933d 
Signed-off-by: Anthony Casagrande <anthony.j.casagrande@intel.com>
@presatish
Copy link
Copy Markdown
Contributor

presatish commented Jun 22, 2023

It would be helpful if examples under https://github.com/EdgeX-Camera-Management/device-usb-camera/blob/fix-discover-panic/cmd/res/devices are updated too. There is no CardName here.

Yes, and not just in the Example App, also here in this repo. https://github.com/edgexfoundry/device-usb-camera/blob/main/cmd/res/devices/general.usb.camera.yaml.example https://github.com/edgexfoundry/device-usb-camera/blob/main/cmd/res/devices/hp.w200.yaml.example

@ajcasagrande , please fix these as part of this PR. THX!

@lenny-intel After looking into the issue more I don't think cardName and serialNumber need to be included in the examples. They are not the required parameters, they can be used if additional checking is required. Initially when I posted my comment I thought they are the required parameters.

@presatish presatish requested a review from lenny-goodell June 22, 2023 20:13
lenny-goodell
lenny-goodell approved these changes Jun 22, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@lenny-goodell lenny-goodell left a comment

Choose a reason for hiding this comment

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

LGTM

@lenny-goodell lenny-goodell linked an issue Jun 22, 2023 that may be closed by this pull request
Panic when device is missing CardName or SerialNumber (Minnesota) #263
Closed
lenny-goodell
lenny-goodell requested changes Jun 22, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@lenny-goodell lenny-goodell left a comment

Choose a reason for hiding this comment

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

Still need to update the example device files to have the work around

lenny-goodell
lenny-goodell approved these changes Jun 23, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@lenny-goodell lenny-goodell left a comment

Choose a reason for hiding this comment

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

LGTM, ok with not updating the examples since the extras are optional.

@presatish presatish merged commit c5a4cd9 into edgexfoundry:main Jun 23, 2023
@presatish presatish deleted the fix-discover-panic branch June 23, 2023 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@presatish presatish presatish left review comments

@lenny-goodell lenny-goodell lenny-goodell approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Panic when device is missing CardName or SerialNumber (Minnesota)

3 participants

@ajcasagrande @lenny-goodell @presatish