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

Align with Commonalities subscription-model by using sink and sinkCredentials #47

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

jgarciahospital
Copy link
Collaborator

What type of PR is this?

Add one of the following kinds:

  • correction
  • enhancement/feature
  • cleanup
  • documentation

What this PR does / why we need it:

Fixing small code and documentation typos and including alignment of "$.sink" and "$.sinkCredentials" properties.

Which issue(s) this PR fixes:

Fixes #38

…on typos and including alignment of "$.sink" and "$.sinkCredentials" properties.

Fixing small code and documentation typos and including  alignment of "$.sink" and "$.sinkCredentials" properties.
@rartych
Copy link

rartych commented Sep 5, 2024

In my view it is not the subscription, but asynchronous response - therefore I opened the issue: camaraproject/Commonalities#297
If we use subscription guidelines the the response should be in Cloudevents format.
Since there is no guidelines I am also fine with the proposed changes.

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.

Proposal to add 3 400 error and that will be good for me.
I saw @rartych comment - agreed that we can align the event itself after the meta release as ti is probably too late to adjust now.

code: "INVALID_ARGUMENT"
message: "Client specified an invalid argument, request body or query param"
code: 'INVALID_ARGUMENT'
message: 'Client specified an invalid argument, request body or query param'
Copy link
Collaborator

@bigludo7 bigludo7 Sep 5, 2024

Choose a reason for hiding this comment

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

What about adding also to be fully aligned with commonalities for the notification credential?

           GENERIC_400_INVALID_PROTOCOL:
              value:
                status: 400
                code: "INVALID_PROTOCOL"
                message: "Only HTTP is supported"
            GENERIC_400_INVALID_CREDENTIAL:
              value:
                status: 400
                code: "INVALID_CREDENTIAL"
                message: "Only Access token is supported"
            GENERIC_400_INVALID_TOKEN:
              value:
                status: 400
                code: "INVALID_TOKEN"
                message: "Only bearer token is supported"`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hope it's solved now, added (except Invalid_protocol, not sure where it comes from) and also added generic missing 410

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

@jgarciahospital
Copy link
Collaborator Author

Thanks, @bigludo7 . @sachinvodafone could you please validate for merging?

@jgarciahospital jgarciahospital merged commit 9c2bdd7 into main Sep 10, 2024
1 check passed
@jgarciahospital jgarciahospital deleted the jgarciahospital-patch-6 branch September 10, 2024 09:34
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.

Validate API code and align with commonalities (meta-release item 1-2-3)
4 participants