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

geofencing: Alignment of errors with Commonalities #223

Conversation

maxl2287
Copy link
Contributor

@maxl2287 maxl2287 commented Jul 2, 2024

What type of PR is this?

Add one of the following kinds:

  • enhancement/feature

What this PR does / why we need it:

Alignement of errors according to Commonalities guideline.

Which issue(s) this PR fixes:

Fixes #222

Changelog input

 release-note

- Aligned error code list with Commonalities for geofencing API

@maxl2287 maxl2287 self-assigned this Jul 2, 2024
@maxl2287 maxl2287 added the enhancement New feature or request label Jul 2, 2024
bigludo7
bigludo7 previously approved these changes Jul 3, 2024
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

@maxl2287 maxl2287 added the Fall24 Meta-release Fall24 label Jul 3, 2024
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 would reconsider adding explicit mention to errors such as 405, 406, 409, 410, 412, 415, 429, which are not part of the core of the API logic, and may be implemented by previous layers (access control, etc). This has been also discussed in QOD

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.

Hello @maxl2287
This is not aligned with current PR in Commonalities; This one should be merged soon (we have a commonalitie meeting today) so I suggest to wait for alignement.

@maxl2287 maxl2287 requested a review from bigludo7 July 23, 2024 10:02
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.

As I try to be consistent :) :) same comments that for Device Status subscription API.

code/API_definitions/geofencing-subscriptions.yaml Outdated Show resolved Hide resolved
code/API_definitions/geofencing-subscriptions.yaml Outdated Show resolved Hide resolved
@maxl2287 maxl2287 requested a review from bigludo7 July 23, 2024 14:02
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 Max
Look good for me.

@maxl2287
Copy link
Contributor Author

@akoshunyadi @jlurien @bigludo7 same discussion here like in camaraproject/DeviceStatus#193 (comment)

We need now a clear decission:
Either we use all of the errorcodes from the template (incl. e.g. HTTP-410 or just the API relevant codes.)

Wdyt?

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. IMO we should restrict the codes to the relevant ones for the spec, but the specific list should be discussed in Commonalities and the guidelines and artifacts be adjusted.

@jlurien jlurien merged commit 3a082db into camaraproject:main Jul 30, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Fall24 Meta-release Fall24
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align Geofencing with guidelines for error scenarios
3 participants