-
Notifications
You must be signed in to change notification settings - Fork 60
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
Rename properties to new terms #129
Rename properties to new terms #129
Conversation
jlurien
commented
Mar 9, 2023
- Applied lowerCamelCase for property names, UpperCamelCase for schema names
- Updated API_documentation as well. This is a redundant effort that hopefully we don't have to maintain in the future, as being discussed in Revisit API documentation template to reduce redundancy WorkingGroups#164
- In a separate PR we should enhance descriptions, once Glossary definitions are consolidated.
- Applied lowerCamelCase for property names, UpperCamelCase for schema names - Updated API_documentation as well. This is a redundant effort that hopefully we don't have to maintain in the future, as being discussed in camaraproject/WorkingGroups#164 - In a separate PR we should enhance descriptions, once Glossary definitions are consolidated.
We should wait with this PR until camaraproject/WorkingGroups#157 is finally decided with a vote as requested by Telefonica. |
Guess we are ready to due these changes now as the guidelines are also updated. @eric-murray @jlurien @patrice-conil @sfnuser @akoshunyadi would you have a look and approve as well? |
code/API_definitions/qod-api.yaml
Outdated
@@ -63,41 +63,41 @@ paths: | |||
code: INVALID_ARGUMENT | |||
message: "Schema validation failed at ..." | |||
MsisdnRequired: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we update this line too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in ed3bbdc
@@ -26,14 +26,14 @@ Security access keys such as OAuth 2.0 client credentials used by client applica | |||
**QoS profiles and QoS profile labels** | |||
Latency or throughput requirements of the application mapped to relevant QoS profile class. | |||
|
|||
**Identifier for the the user equipment (UE)** | |||
At least one identifier for the user equipment out of four options: IPv4 address, IPv6 address, MSISDN, or external ID [[5]](#5) assigned by the mobile network operator for the user equipment | |||
**Identifier for the the device** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: the the
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in fe2cef9
**Identifier for the the user equipment (UE)** | ||
At least one identifier for the user equipment out of four options: IPv4 address, IPv6 address, MSISDN, or external ID [[5]](#5) assigned by the mobile network operator for the user equipment | ||
**Identifier for the the device** | ||
At least one identifier for the device (user equipment) out of four options: IPv4 address, IPv6 address, Phone number, or Network Access Identifier [[5]](#5) assigned by the mobile network operator for the device. | ||
|
||
**Identifier for the application server (AS)** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AS is not used anymore, more occurrences are affected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in fe2cef9 (plus replaced one remaining occurence of UE).
@jlurien will you resolve the comments or should I? |
MsisdnRequired replaced with PhoneNumber required.
Corrected type and remaining occurences of AS and UE.
AS @jlurien seems not be available I have addressed the comments above. |
@hdamker - Cheers. Are we planning to update the commit in two other places (line 83 & 95) as well? |
Further updates of error examples in lines 71, 77, 83, 89, 95
@sfnuser :
Thanks for the hint ... have done further updates of error examples in lines 71, 77, 83, 89, 95 to get all the error examples consistent with the new property terms. Please have a view. |
Thanks. Looks good to me. |
code/API_definitions/qod-api.yaml
Outdated
value: | ||
status: 400 | ||
code: INVALID_ARGUMENT | ||
message: "Expected property is missing: qos" | ||
message: "Expected property is missing: qosProfile" | ||
UePortsRangesNotAllowed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hdamker - are we planning to update this one too?
code/API_definitions/qod-api.yaml
Outdated
value: | ||
status: 400 | ||
code: INVALID_ARGUMENT | ||
message: "Expected property is missing: ueId.ipv4addr or ueId.ipv6addr" | ||
message: "Expected property is missing: device.ipv4Address or device.ipv6Address" | ||
UePortsRequired: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hdamker - are we planning to update this one too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully we have now found all occurence which need to be replaced.