x-correlator_guideline_format_convention by PedroDiez · Pull Request #194 · camaraproject/Commonalities

PedroDiez · 2024-05-02T19:28:51Z

What type of PR is this?

correction

What this PR does / why we need it:

This PR covers the x-correlator naming guideline adoption.
Also given format to CAMARA_common.yaml (2-space indentation)

Documentation impacted (some doubts commented within PR):

Which issue(s) this PR fixes:

Fixes #191

Special notes for reviewers:

Some notes:

To be merged after guideline_for_datacontenttype_cloudevent_concept_as_enum #193
Should we take advantage of this PR and align error exceptions for POST /notifications? To align with Create subscription-notification-template.yaml #189. Commented within PR
Removed Exception 415
Refactor bearer format to bearerFormat: "{$request.body#/sinkCredential.credentialType}"

Changelog input

 release-note

Additional documentation

N/A

PedroDiez · 2024-05-02T19:30:10Z

cc @jlurien

artifacts/CAMARA_common.yaml

PedroDiez · 2024-05-02T19:37:47Z

artifacts/notification-as-cloud-event.yaml

+              $ref: "#/components/headers/x-correlator"
          content:
            application/json:
              schema:


Regarding Exceptions alignment with PR#189:

400, 401, 403, 500, 503 are clear

415, 429 think makes sense to have in PR#189 as well

410 not sure about UC for 410. In case we keep it, we should align in PR#189 as well

cc @shilpa-padgaonkar, @patrice-conil, @bigludo7, @rartych, @jlurien

Ok for me to add 415 & 429 in PR#189 event if I'm bit dubious about the UC. I will do.

For 410 I'm more in favor to not add it but will follow your guidance.

My understanding:
415 - would be the case when API Consumer (Server Side in this flow) does not support "application/json" -> This in principle should never happen because is a guideline of CloudEvent CAMARA proifle
429 - would be the case when API Consumer (Server Side in this flow) receiving many requests over its threshold (could happen)

@patrice-conil Do you have context about why 410, 415, 429 were included in the initial proposal? 339a8e2

410 - The unique case I think it may apply is the case of that callback no longer available. I mean the Server side decided to switch-off that endpoint. This would be differnet to a 503, because in 503 endpoint exists but is not available temporarily (regular exception for maintenance tasks)

So following your explanation @PedroDiez I understand that we should have 429 & 410 and drop 415.

Hi @PedroDiez,
My understanding:
410 was introduced to handle cases where the callback URL is no longer available.
415 is what the server side should respond if Content-type or Accept are not "application/json" (should never happen) .
429 could be used to handle congestion on the callback endpoint.

Hi @patrice-conil, thanks for the clarification! I think we need to align with #189 (POST callback) cc @bigludo7

My view:

Keep 410 for the case the endpoint is no longer available (Not 503 case because that one is temporal). Benefit is to have an indication for Telco Operation teams to enable operations procedure to the Subscriber to say "Please be aware there is a flow no longer working, you will need to generate a new subscription with a new callback or in the case of implicit flow to indicate new callback"

Would take out 415 - Should never happen because developers MUST adopt CAMARA cloudevent profile (if you think/prefer to have for safety/operation reasons, i.e. to advise an App developer that need to support application/json as a warning to adapt its side to be able to interoperate it is fine)

Keep 429

WDYT?

@PedroDiez If we agree that 415 is out, rest of your PR LGTM & I can provide an approval once this change is done.

artifacts/CAMARA_common.yaml

Co-authored-by: Shilpa Padgaonkar <77152136+shilpa-padgaonkar@users.noreply.github.com>

1-space indentation for compliance with current style

…into x-correlator_guideline_format_convention

…thub.com/camaraproject/Commonalities into x-correlator_guideline_format_convention

PedroDiez · 2024-05-29T17:57:15Z

ready for review
cc @shilpa-padgaonkar @rartych @patrice-conil @bigludo7

I have updated bearer format to: "{$request.body#/sinkCredential.credentialType}"

As https://swagger.io/docs/specification/authentication/bearer-authentication/ indicates, is an optional property and a hint for the client. ( # optional, arbitrary value for documentation purposes), so semantically speaking accordingly to CloudEvents model is the concept of credentialType and we are restricting to be only ACCESSTOKEN (to me makes more sense but to have aligment/consensus on that)

If you think it is better to indicate: "{$request.body#/sinkCredential.accessToken}" just because we only allow ACCESSTOKEN so far and the format is "accessToken" (i.e. a string) it is also fine to me.

Just to be transparent about this.

shilpa-padgaonkar

/LGTM

shilpa-padgaonkar · 2024-05-31T09:35:06Z

@patrice-conil or @bigludo7 : Could you kindly review and approve the PR if you find no issues so that we can proceed to merge?

patrice-conil

LGTM

PedroDiez · 2024-05-31T11:24:25Z

If no other comments I will be merging on Monday next week

bigludo7

/LGTM

x-correlator_guideline_format_convention

cf3896f

PedroDiez added the correction correction in documentation label May 2, 2024

PedroDiez self-assigned this May 2, 2024

PedroDiez requested review from RubenBG7, patrice-conil, rartych and shilpa-padgaonkar as code owners May 2, 2024 19:28

PedroDiez requested a review from bigludo7 May 2, 2024 19:29

PedroDiez commented May 2, 2024

View reviewed changes

shilpa-padgaonkar reviewed May 3, 2024

View reviewed changes

artifacts/CAMARA_common.yaml Outdated Show resolved Hide resolved

PedroDiez mentioned this pull request May 17, 2024

Create subscription-notification-template.yaml #189

Merged

PedroDiez and others added 5 commits May 17, 2024 16:38

Update artifacts/CAMARA_common.yaml

f6cc8fd

Co-authored-by: Shilpa Padgaonkar <77152136+shilpa-padgaonkar@users.noreply.github.com>

adding_410_common_yaml_and_fix_typos_in_cloud_event

b0b5906

fix_status_typo

bc75a74

merge_with_main

aaa2e88

Update CAMARA_common.yaml

45fe0de

1-space indentation for compliance with current style

rartych mentioned this pull request May 27, 2024

Commonalities release 0.4.0 scope #175

Closed

PedroDiez added 4 commits May 29, 2024 18:59

Merge branch 'main' of https://github.com/camaraproject/Commonalities …

f6f43a5

…into x-correlator_guideline_format_convention

delete_415_error_update_sink_info

be90a65

Merge branch 'x-correlator_guideline_format_convention' of https://gi…

6dcfdea

…thub.com/camaraproject/Commonalities into x-correlator_guideline_format_convention

Trigger CI

afe30cd

PedroDiez requested a review from shilpa-padgaonkar May 29, 2024 17:57

shilpa-padgaonkar approved these changes May 31, 2024

View reviewed changes

patrice-conil approved these changes May 31, 2024

View reviewed changes

bigludo7 approved these changes May 31, 2024

View reviewed changes

shilpa-padgaonkar mentioned this pull request May 31, 2024

Alignment_with_PR_198 #219

Merged

PedroDiez merged commit e873182 into main Jun 3, 2024

rartych mentioned this pull request Jun 5, 2024

Update of linting ruleset #188

Closed

Kevsy mentioned this pull request Jun 7, 2024

X-Correlator -> x-correlator camaraproject/SimpleEdgeDiscovery#8

Closed

rartych deleted the x-correlator_guideline_format_convention branch October 9, 2024 11:49

Conversation

PedroDiez commented May 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for reviewers:

Changelog input

Additional documentation

Uh oh!

PedroDiez commented May 2, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PedroDiez May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PedroDiez commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shilpa-padgaonkar left a comment

Choose a reason for hiding this comment

Uh oh!

shilpa-padgaonkar commented May 31, 2024

Uh oh!

patrice-conil left a comment

Choose a reason for hiding this comment

Uh oh!

PedroDiez commented May 31, 2024

Uh oh!

bigludo7 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

PedroDiez commented May 2, 2024 •

edited

Loading

PedroDiez May 3, 2024 •

edited

Loading

PedroDiez commented May 29, 2024 •

edited

Loading