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

adding MC ATP yaml #8

Merged
merged 17 commits into from
Dec 29, 2022
Merged

Conversation

DT-DawidWroblewski
Copy link
Collaborator

No description provided.

DT-DawidWroblewski and others added 9 commits November 18, 2022 10:20

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
Copy link

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

This openapi definition is incorrect, you need to check semantic and syntax erors using https://editor.swagger.io/.
For axample in schemas you need to group "required" attribute in a separate section.

    atpTimestamp:
      type: object
      properties:
        simChange:
          type: string
          required: true
          example: 'simChange: 2022-12-06'
        isUncontidionalCallDivertActive:
          type: string
          required: false

should be

    atpTimestamp:
      type: object
      properties:
        simChange:
          type: string
          example: 'simChange: 2022-12-06'
        isUncontidionalCallDivertActive:
          type: string
        required: 
          - simChange

I can help to fix these issues.

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 made a quick review with some comments for your consideration.
You can check also these errors at https://editor.swagger.io/#/:

Semantic error at security.0
Security requirements must match a security definition
Jump to line 118
Semantic error at security.1
Security requirements must match a security definition
Jump to line 119

code/API_definitions/MC_ATP.yaml Outdated Show resolved Hide resolved
code/API_definitions/MC_ATP.yaml Show resolved Hide resolved
tags:
- name: Mobile Connect ATP
paths:
/token:
Copy link
Collaborator

Choose a reason for hiding this comment

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

/token endpoint is part of the authentication flow and should not be here. All paths in this yaml file are supposed to have a common server and base path, so implementation could not hace distinct servers for /token and other resources

Copy link
Collaborator

Choose a reason for hiding this comment

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

In CAMARA numberVerify proposal we have endpoints for /authorize & /token. We should have same pattern everywhere.
As this proposal is based on MC I'm fine with these endpoints but probably we could add a comment to indicate that they could be in a distinct server as mentionned by @jlurien

BTW, @jlurien in QoD API we have in the same swagger for both /sessions & notifications and they will not be implemented in same server.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should leave this comment inside swagger or in MD file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would document it in both. Problems with yaml may rise when some client library try to genere code directly from yaml definition. We should be more careful with these aspects across all APIs when final v1 versions are released.

- active
- inactive
required:
- simSwap
Copy link
Collaborator

Choose a reason for hiding this comment

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

this property is not listed above

code/API_definitions/MC_ATP.yaml Show resolved Hide resolved
@DT-DawidWroblewski
Copy link
Collaborator Author

DT-DawidWroblewski commented Dec 15, 2022 via email

patrice-conil
patrice-conil previously approved these changes Dec 16, 2022
@DT-DawidWroblewski
Copy link
Collaborator Author

@bigludo7 can you please review the code today and approve? thx!

@DT-DawidWroblewski
Copy link
Collaborator Author

@bigludo7 please review the code - thx!

documentation/API_documentation/API_definition.md Outdated Show resolved Hide resolved
code/API_definitions/MC_ATP.yaml Outdated Show resolved Hide resolved
tags:
- name: Mobile Connect ATP
paths:
/token:
Copy link
Collaborator

Choose a reason for hiding this comment

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

In CAMARA numberVerify proposal we have endpoints for /authorize & /token. We should have same pattern everywhere.
As this proposal is based on MC I'm fine with these endpoints but probably we could add a comment to indicate that they could be in a distinct server as mentionned by @jlurien

BTW, @jlurien in QoD API we have in the same swagger for both /sessions & notifications and they will not be implemented in same server.

@jlurien
Copy link
Collaborator

jlurien commented Dec 28, 2022

@bigludo7 Regarding "in QoD API we have in the same swagger for both /sessions & notifications and they will not be implemented in same server."

indeed it is a different server. In that case we opted to add a disclaimer in description:

      description: |
        Important: this endpoint is to be implemented by the API consumer.
        The QoD server will call this endpoint whenever any network related event occurs.
        Currently only SESSION_TERMINATED event is implemented. Any other network events are ignored.

DT-DawidWroblewski and others added 2 commits December 29, 2022 09:02
Co-authored-by: Jose Luis Urien <jlurien@gmail.com>
Co-authored-by: Jose Luis Urien <jlurien@gmail.com>
@DT-DawidWroblewski DT-DawidWroblewski requested review from bigludo7 and removed request for patrice-conil December 29, 2022 08:10
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.

Look good for me as a first version.
Agreed with @jlurien about /authorize & /token endpoints but should tackle this question globally for all API. This is probably a point worth to be discussed in Commonalities --> we need an issue for that.

@DT-DawidWroblewski DT-DawidWroblewski merged commit 4f9aa39 into camaraproject:main Dec 29, 2022
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.

None yet

4 participants