Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(data): update validateEvent topic check #4345

Closed
wants to merge 1 commit into from
Closed

feat(data): update validateEvent topic check #4345

wants to merge 1 commit into from

Conversation

chr1shung
Copy link

@chr1shung chr1shung commented Feb 13, 2023

event topic from device service will be:
<prefix>/<service-name>/<profile-name>/<device-name>/<source-name>

corresponding to edgexfoundry/device-sdk-go#1261

Signed-off-by: Chris Hung chris@iotechsys.com

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

New Dependency Instructions (If applicable)

event topic from device service will be:
<prefix>/<service-name>/<profile-name>/<device-name>/<source-name>

corresponding to edgexfoundry/device-sdk-go#1261

Signed-off-by: Chris Hung <chris@iotechsys.com>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@chr1shung chr1shung marked this pull request as draft February 13, 2023 08:36
@chr1shung
Copy link
Author

chr1shung commented Feb 13, 2023

@lenny-intel according to document app-service expects event topic to have the same suffix:
https://docs.edgexfoundry.org/3.0/microservices/general/messagebus/#multi-level-topics-and-wildcards

However if event is added by REST API, coredata does not know the device service name:
https://github.com/edgexfoundry/edgex-go/blob/main/internal/core/data/application/event.go#L84

Do we allow this inconsistency ?
edgex/events/device/<service>/<profile>/<device>/<source> vs edgex/events/core/<profile>/<device>/<source>

or maybe we update the event topic published by coredata to edgex/events/core/core-data/<profile>/<device>/<source> ?

Copy link
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
Copy link
Member

recheck

@lenny-goodell
Copy link
Member

lenny-goodell commented Feb 14, 2023

However if event is added by REST API, coredata does not know the device service name:

@hahattan , the Add Event REST API needs to be updated to include the DeviceServiceName

@chr1shung
Copy link
Author

However if event is added by REST API, coredata does not know the device service name:

@hahattan , the Add Event REST API needs to be updated to include the DeviceServiceName

@lenny-intel REST endpoint only includes device name, profile name and source name.
Do we have any other way to retrieve device service name in this case ?

@cloudxxx8
Copy link
Member

@hahattan after the discussion in the Core WG meeting on 15th-Feb-2023, we should modify the REST API to fit this change
/event/{profileName}/{deviceName}/{sourceName}
it will become /event/{serviceName}/{profileName}/{deviceName}/{sourceName}

After this API change, you can get the device service name from REST API

@lenny-goodell
Copy link
Member

lenny-goodell commented Feb 15, 2023

@hahattan , @cloudxxx8 , FYI may PR here #4348 took care of this except for updating the swagger. This PR can be closed.

@cherrycl , FYI for TAF tests

@chr1shung chr1shung closed this Feb 15, 2023
fields := strings.Split(messageTopic, "/")

// assumes a non-empty base topic with /profileName/deviceName/sourceName appended by publisher
if len(fields) < 4 {
if len(fields) < 5 {
Copy link
Author

Choose a reason for hiding this comment

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

@lenny-intel Do you think we still need to check this ? I didn't see the change in #4348

Copy link
Member

Choose a reason for hiding this comment

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

I believe we need this check, and update the comment in line 120
however, why doesn't it if len(fields) < 6?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants