camaraproject / DeviceLocation

Repository to describe, develop, document and test the DeviceLocation API family
Apache License 2.0
21 stars 32 forks source link

New API for location subscriptions #74

Closed chia2017 closed 11 months ago

chia2017 commented 1 year ago

What type of PR is this?

enhancement/feature

What this PR does / why we need it:

Create a new API for creating subscriptions for location based events

Which issue(s) this PR fixes:

This PR is related to issue #18 Subscription for Location Area

Fixes #

Special notes for reviewers:

Changelog input

 release-note

Additional documentation

This section can be blank.

docs
jlurien commented 1 year ago

@chia2017 you still have to define the details in the specific events being notified, that is the device and area.

        eventDetail:
          type: object -> here instead of object
chia2017 commented 1 year ago

Rename api to geofencing

Hi, I renamed the api two weeks ago before I went to vacation. So I did not recieve any new comment about review, If you think everything is fine you can merge this PR, but first it need two approvers. Anyway I am available if you have any comment. Best, Sadeghi

jlurien commented 1 year ago

Rename api to geofencing

Hi, I renamed the api two weeks ago before I went to vacation. So I did not recieve any new comment about review, If you think everything is fine you can merge this PR, but first it need two approvers. Anyway I am available if you have any comment. Best, Sadeghi

Hi Sadeghi. I made another review. I think it is almost ready. Once @bigludo7 is back and make a further check we can merge. Thanks!

bigludo7 commented 1 year ago

@chia2017 @jlurien Thanks for the effort - It looks good for me and thanks to have fixed my previous comments. Once José comments resolved we should be good for merging

chia2017 commented 1 year ago

I updated the API and applied all last comments that you wrote. If you have no more comment you can merge it and we can close the task. If you have further request you can contact my colleague Maximilian Laue, since I am not working with this project and I switched to the new project. Best, Sadeghi

maxl2287 commented 1 year ago

I updated the API and applied all last comments that you wrote. If you have no more comment you can merge it and we can close the task. If you have further request you can contact my colleague Maximilian Laue, since I am not working with this project and I switched to the new project. Best, Sadeghi

@bigludo7 & @jlurien:

Could you please review again. When anything needs to be adapted then I will fix it asap.

Thanks! 🚀

bigludo7 commented 1 year ago

Thanks @chia2017 for the effort and good luck for your new project @maxl2287 As of now we cannot approve because following DT suggestion (and CAMARA team approval) the event notification should be aligned with the CloudEvents. This update is describe here: https://github.com/camaraproject/Commonalities/pull/56 and right now is in team review.

My recommendation is to wait for approval before to adjust your code. Thanks

jlurien commented 11 months ago

Now that https://github.com/camaraproject/Commonalities/pull/56 has been merged, we should update this PR accordingly @maxl2287, @akoshunyadi. Thanks

maxl2287 commented 11 months ago

I created PR #98 to align on CloudEvents based on these commits here. In the fact that @chia2017 is not part of the CAMARA project anymore and I am not allowed to add commits to his repository.

bigludo7 commented 11 months ago

This feature is now followed here: https://github.com/camaraproject/DeviceLocation/pull/98