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

Alignement of event-subscription-template.yaml with CAMARA_common.yaml for the errors #251

Merged
merged 8 commits into from
Jul 22, 2024

Conversation

bigludo7
Copy link
Collaborator

What type of PR is this?

Add one of the following kinds:

  • correction

What this PR does / why we need it:

Alignement of artifacts/camara-cloudevents/event-subscription-template.yaml with artifacts/CAMARA_common.yaml for the errors

cc: @PedroDiez @shilpa-padgaonkar @akoshunyadi @maxl2287

Which issue(s) this PR fixes:

Fixes #250

Special notes for reviewers:

This is an urgent issue as required for this meta release for some of our API.

Changelog input

 release-note
-Alignement of artifacts/camara-cloudevents/event-subscription-template.yaml with artifacts/CAMARA_common.yaml for the errors

Additional documentation

This section can be blank.

docs

bigludo7 and others added 4 commits July 18, 2024 08:24
Co-authored-by: Pedro Díez García <pedro.diezgarcia@telefonica.com>
Co-authored-by: Pedro Díez García <pedro.diezgarcia@telefonica.com>
Co-authored-by: Pedro Díez García <pedro.diezgarcia@telefonica.com>
@bigludo7
Copy link
Collaborator Author

Thanks a lot @PedroDiez for the review. I've took into consideration all your comments.

TokenMismatch:
code: PERMISSION_DENIED
message: Client does not have sufficient permissions to perform this action.
GENERIC_403_SUBSCRIPTION_MISMATCH:

Choose a reason for hiding this comment

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

I think SUBSCRIPTION_MISMATCH doesn't belong to Generic403, it's subscription specific
Can't we have only 1 403 type like SubscriptionPermissionDenied with 3 examples (permissiondenied, invalidtoken, subscription mismatch), and remove Generic403?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@akoshunyadi I used CreateSubscriptionPermissionDenied403 only for the POST /subscriptions and I have 403_INVALID_TOKEN_CONTEXT specific for the POST
Generic403 is for the GET & DELETE and here we have the GENERIC_403_SUBSCRIPTION_MISMATCH which applies only for GET & DELETE and not POST

Choose a reason for hiding this comment

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

For me GenericXXX are types which we could put into/take from the common yaml and use for all APIs, because they don't contain any API specific hints. So I think we can have multiple 403 types here but if *Generic then it should only have generic examples.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bigludo7 I think GENERIC_403_SUBSCRIPTION_MISMATCH is to be used within CreateSubscriptionPermissionDenied403 (Line 896) and within "Generic 403" (Line 875) also applies GENERIC_403_INVALID_TOKEN_CONTEXT

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

humm...hope i was able to follow you
I've defined SubscriptionPermissionDenied403 for all subscription related 403 for the operations and I kept Generic403 as response possible for the callback.

status: 415
code: UNSUPPORTED_MEDIA_TYPE
message: The server refuses to accept the request because the payload format is in an unsupported format
Generic422:

Choose a reason for hiding this comment

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

I think it should be named specifically, since it contains subscription specific examples

Copy link
Collaborator Author

@bigludo7 bigludo7 Jul 18, 2024

Choose a reason for hiding this comment

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

hummm so I not get it because when I read in the CAMARA_common.yam that we can have in Generic422:

            GENERIC_422_{{SPECIFIC_CODE}}:
              description: Any semantic condition associated to business logic, specifically related to a field or data structure
              value:
                status: 422
                code: { { SPECIFIC_CODE } }
                message: { { SPECIFIC_CODE_MESSAGE } }

(lines 373) I was thinking this is a placeholder for specific code.
What is your understanding @akoshunyadi ?

Choose a reason for hiding this comment

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

yes, but I still have the problem that we will have many e.g. Generic422 over all Camara APIs but there are not the same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've changed to CreateSubscriptionUnprocessableEntity422 ;)
Hope that fine for @PedroDiez

"409":
$ref: "#/components/responses/Generic409"
"415":
$ref: "#/components/responses/Generic415"
"422":
$ref: "#/components/responses/CreateSubscription422"
$ref: "#/components/responses/Generic422"
"429":
$ref: "#/components/responses/Generic429"
"500":

Choose a reason for hiding this comment

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

How about other errorcodes in the guideline; e.g. 405,406,501 , I think those ones would make a sense here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did not add them because they were not in the initial yaml.
Regarding 405 & 406 I'm fine to add them. @PedroDiez WDYT?
Regarding 501 with @PedroDiez we think that this API will "not work" if all operation are not implemented. Indeed, it you provide subscription you must provide POST/GET/DELETE

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer not doing now. And it is fair point of discussion.

Because there is no common guideline about to mention explicitly HTTP codes that are usually managed at API-GW Infras. We need a guideline first to agree on which HTTP codes are traversal to be explicitly mention in every API and which not (as assumed that any implementation will manage it accordingly). If not we will again have misaligment among APIs.

To me is an issue for discussion for the next MetaRelease Spring 25

Choose a reason for hiding this comment

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

Fine for me to discuss it later.

Copy link

@akoshunyadi akoshunyadi 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

@PedroDiez PedroDiez left a comment

Choose a reason for hiding this comment

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

LGTM. Points covered

Copy link
Collaborator

@patrice-conil patrice-conil 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
Copy link
Collaborator Author

Hello @rartych, @shilpa-padgaonkar
I think we're good to go on this one :)

Copy link
Collaborator

@shilpa-padgaonkar shilpa-padgaonkar left a comment

Choose a reason for hiding this comment

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

/LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alignement of event-subscription-template.yaml with CAMARA_common.yaml for the errors
6 participants