Conversation
fernandopradocabrillo
requested review from
FabrizioMoggio and
chinaunicomyangfan
as code owners
Do you mean "aligned with Spring25"? |
|
has this release
Got it (#18 (comment)). Ok, for Fall24. |
tanjadegroot
left a comment
tanjadegroot
left a comment
There was a problem hiding this comment.
Review done - please handle comments as you see fit
LGTM from Release Management
There was a problem hiding this comment.
line 69: termsOfService field and contact (line 70-71) fields should be removed
under the version field add camara specific extension: "x-camara-commonalities: 0.5" (or 0.4 if you stay on the Fall24 baseline)
| This release contains the definition and documentation of | ||
| * most-frequent-location v0.1.0 | ||
|
|
||
| The API definition(s) are based on |
There was a problem hiding this comment.
This is the base of the Fall24 release. Are you planning to align to the Spring25 Commonalities (0.5.0-rc.1) and ICM (0.3.0-rc.1) ? if so, you need to update the references here, but also do a number of PRs to align and then add the changes wrt the new baseline to the changelog, and add the compare with r1.1)
There was a problem hiding this comment.
From TEF side we need to formalise a public release with Fall24 guidelines. We have no plans to align with Spring25 and I don't see initiatives from other companies to do so.
There was a problem hiding this comment.
ok, thanks Fernando, that is clear
There was a problem hiding this comment.
line 11: the concept of "API family" has been deprecated. Change to "APIs".
line 16: change "customer" to "API Consumer"
the other occurrences of "customer" in this description field should be changed to "end-user" I think.
line 18: the "DeviceVisitLocation" query does not seem to exist - remove this part.
| version: 0.1.0 | ||
| externalDocs: | ||
| description: Product documentation at CAMARA | ||
| url: https://github.com/camaraproject/ |
There was a problem hiding this comment.
Add the repository name "LocationInsights" at the end of the URL
There was a problem hiding this comment.
-
Instead of defining your own geographical datatype, I recommend using the common schema Area as defined in https://github.com/camaraproject/Commonalities/blob/r2.1/artifacts/CAMARA_common.yaml
-
Is the use of postal code an expressed requirement ? the API could be much simpler if it just uses Area.
Postal codes have many different formats in different countries, sometimes map weirdly to geographical locations, imply an additional transformation. etc. -
the text includes reference to the usage of the location of antennas - I would remove such implementation details from the API documentation, as the Area can be supported in many different ways. Also this is very network specific (which is against CAMARA principles) and limits the APIs to mobile networks (while a wider usage should be possible)
decision up o the team
There was a problem hiding this comment.
Yes, this is a topic that has been raised before, but at the time, the area object didn't meet the exact requirements for the product. This is something to include in the backlog to update.
For now, the idea is to release a freeze version v0.1.0 as it is in the -rc and then begin to work on improvements. All the testing has been done with the -rc version and at least from our side, is the version we need. Happy to discuss improvements for future versions.
There was a problem hiding this comment.
line 52: please check Commonalities guidelines for mandatory template text on auth.
You may also decided to align the Commonalities 0.5 error codes
There was a problem hiding this comment.
line 11: "service provider" --> "application provider" to avoid ambiguity
"customer" --> "end-user"
line 24: "Device" definition: suggest reusing the definition from the CAMARA Glossary
lines 32, 98, 112, 121, 131, 142: "how often a device ..." raises the expectation of getting a count.
Suggest to use instead "the frequency with which a device ..."
|
Hi @tanjadegroot , thanks a lot for the suggestions, I've taken almost all of them, I just left aside those referring to alignments with Comm v0.5 or ICM 0.3. We'll stick to Fall24 for now but surely improve the api for next versions. |
|
Note: I recommend to create an issue in the backlog for the Area usage discussion (if you plan to use it) I updated your API release tracker with the new release, included the review issue and aligned the tracker to the latest version. |
3 participants
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
PR to formalize release candidate version v0.1.0-rc into a final initial version v0.1.0.
This API is still aligned with meta Fall24 and no new changes have been included.