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: add subscriptionMaxEvents for maximum number of notifications (geofencing) #163

Conversation

maxl2287
Copy link
Contributor

@maxl2287 maxl2287 commented Mar 5, 2024

What type of PR is this?

Add one of the following kinds:

  • enhancement/feature
  • correction

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #111
Fixes #161

Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

Thanks for the quick update @maxl2287
For the update look good for me but looking for @jlurien view on the version. Shifting to V0.2 for this API makes sense but we want to close Release V0.2 (for all 3 APIs) so perhaps we have to wait to merge this one. See @jlurien comment here: #160

@maxl2287
Copy link
Contributor Author

maxl2287 commented Mar 6, 2024

@bigludo7 v0.2.0 for DeviceLocation was released yesterday.
This is the new version for Geofencing API, which will be v0.2.0 for this API next, but will be included in the next DeviceLocation release v0.3.0.

That's why I used here v0.2.0-wip.

@jlurien
Copy link
Collaborator

jlurien commented Mar 8, 2024

Yes, until we have clear rules for API family versioning, I think we should continue with our current one, so new updates on geofencing should be part of 0.2.0-wip

@maxl2287 maxl2287 requested a review from bigludo7 March 8, 2024 09:50
Copy link
Collaborator

@jlurien jlurien left a comment

Choose a reason for hiding this comment

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

I suggest to consolidate #162 on this to avoid with conflicts

code/API_definitions/geofencing.yaml Outdated Show resolved Hide resolved
code/API_definitions/geofencing.yaml Outdated Show resolved Hide resolved
code/API_definitions/geofencing.yaml Show resolved Hide resolved
code/API_definitions/geofencing.yaml Outdated Show resolved Hide resolved
@maxl2287
Copy link
Contributor Author

maxl2287 commented Mar 8, 2024

thanks for the review.
I will adapt the changes and close the other one

Copy link
Collaborator

@jlurien jlurien left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

LGTM

@bigludo7 bigludo7 merged commit 7405958 into camaraproject:main Mar 12, 2024
@maxl2287 maxl2287 deleted the feature/add-subscriptionMaxEvents-geofencing branch March 12, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants