-
Notifications
You must be signed in to change notification settings - Fork 5
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
Release r1.1 (Release Candidate) #71
Conversation
Hi @PedroDiez thanks for creating the release PR. I've created for the review camaraproject/ReleaseManagement#138, but seems we need to wait until the open PRs are merged. The release PR
Minor point: |
Hi @hdamker, Thanks for the comments.
|
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.
this PR needs updates, and the other PRs should be merged before this on. this should be the last PR to merge.
Some updates needed and PRs still to be merged before the release PR can be merged. |
I will wait to make updates until merge of PRs
So i can also address some minor points (e.g. version and servers.url adaptation) |
Please try to have that done this week. |
Have manage comments. Waiting to merge #69 first and align branch |
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.
Mandatory template for 2 legged Auth is missing
PhoneNumber related errors (as defined by commonalities guidelines and in https://github.com/camaraproject/Commonalities/blob/r2.2/artifacts/CAMARA_common.yaml) seem to be missing
Also some further comments on API consumer facing documentation. API provider focused documentation should be in supporting documents.
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.
Line 10. repleace "API family" by just "APIs", as the API family concept no longer exists in CAMARA.
If this API and its repository is part of a Sub Project? you should mention it here as well
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.
Ok, changed to "APIs".
In this case the Sub-proyect equals to The CAMARA Repository
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.
Line 14.: "telco service providers have the opportunity to provide 3rd parties with the following capability"
Suggest to change to: "API providers offer API consumers the following capability"
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.
ok, done
README.md
Outdated
- [View it on Swagger Editor](https://editor.swagger.io/?url=https://raw.githubusercontent.com/camaraproject/BlockchainPublicAddress/release-0.1.0/code/API_definitions/blockchain_public_address.yaml) | ||
* All releases and pre-releases are available here https://github.com/camaraproject/BlockchainPublicAddress/releases | ||
* For changes see [CHANGELOG.md](https://github.com/camaraproject/BlockchainPublicAddress/blob/main/CHANGELOG.md) | ||
<!-- Use/uncomment one or multiple the following options --> |
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.
You may remove the commented lines (as you used them below
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.
ok, done
README.md
Outdated
<!-- For changes see [CHANGELOG.md](/CHANGELOG.md) --> | ||
* Note: Please be aware that the project will have updates to the main branch. There are no compatibility guarantees associated with code in any branch, including main, until a new release is created. For example, changes may be reverted before a release is created. **For best results, use the latest available release**. | ||
|
||
* **The Release [r1.1 - rc](https://github.com/camaraproject/BlockchainPublicAddress/blob/main/CHANGELOG.md#r1.1---rc) for the Blockchain Public Address APIs Family is available.** |
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.
there should never be a "main" in a link, only the release (in this case "r1.1") so this link should be:
** The Release r1.1 for the Blockchain Public Address APIs is available.
and I would put it just under the title.
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.
Just to make it explicit: the link to the release will be https://github.com/camaraproject/BlockchainPublicAddress/releases/tag/r1.1 (and will be valid only after the release got created)
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.
Thanks, i indicated wrong link
README.md
Outdated
* **The Release [r1.1 - rc](https://github.com/camaraproject/BlockchainPublicAddress/blob/main/CHANGELOG.md#r1.1---rc) for the Blockchain Public Address APIs Family is available.** | ||
<br>This is a release candidate. Until the public release there are bug fixes to be expected. The release candidate is suitable for implementors, but it is not recommended to use the API with customers in productive environments. | ||
|
||
* The release **r1.1 - rc** is available in [r1.1](https://github.com/camaraproject/BlockchainPublicAddress/tree/r2.1), and includes the following APIs: |
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.
* The release **r1.1 - rc** is available in [r1.1](https://github.com/camaraproject/BlockchainPublicAddress/tree/r2.1), and includes the following APIs: | |
* The release **r1.1 - rc** is available in [r1.1](https://github.com/camaraproject/BlockchainPublicAddress/tree/r1.1), and includes the following APIs: |
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.
The link to the release will be https://github.com/camaraproject/BlockchainPublicAddress/releases/tag/r1.1
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.
ok, done
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.
line 18: "Notice that the mobile phone number used as input may even not belong to the same Telco Operator exposing the API. It is expected a communication between Telco Operators to resolve the Blockchain Public Address(es). For example a Telco Operator will receive the request, identify the Telco Operator which owns the mobile phone number, and forward the request using a 2-legged approach to contact the other Telco Operator."
This description is focused on the API provider, not the API consumer who does not want to know about the complexity between Telco's etc.
I would drop this whole paragraph
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.
Ok, done, understood the rationale.
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.
Logically line 19 should go before line 17
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.
line 19: Change "sub" to "user
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 will rephrase that.
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.
line 22: before this section, you need tto include the mandatory text from Commonalities from this section:
https://github.com/camaraproject/Commonalities/blob/r2.2/documentation/API-design-guidelines.md#appendix-a-normative-infodescription-template-for-when-user-identification-can-be-from-either-an-access-token-or-explicit-identifier
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.
This is a template issued for APIs that may work in 2/3 legged in order to avoid the API Misuse implicitly exposing Number Verification feature.
The template has not been indicated because it is not the case for Blockchain Public Address. The rationale under it is indicated below:
- Perform a binding is a only 3-legged mode operation, and "phoneNumber" indication is required to enforce the procedure to ensure the binding process
- Remove a binding does not use "phoneNumber" as a input
- Retrieve a list of bindings may work in 2/3legged mode and always requires "phoneNumber" as an input to perform the logic.
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.
on the endpoint description: please make sure that what is written about 2 legged / 3 legged is in synch with the lates commonalities.
You may also explain here any API specific errors returned.
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.
addressed in above comment
Good point Tanja, let me explain below in this thread |
@camaraproject/blockchain-public-address_codeowners - please proceed to release for M3 closure |
@PedroDiez removed myself from the reviewers as @tanjadegroot has already done the review for Release Management |
What type of PR is this?
What this PR does / why we need it:
Blockchain Public Address Release r1.1 (Release Candidate) PR (Spring25)
Which issue(s) this PR fixes:
Fixes #66
Fixes #63
Fixes #65
Fixes #64
Special notes for reviewers:
The following PRs MUST be merged prior to this one:
Update User Stories filename in #67
API Spec aligment with Commonalities and ICM in #69
Basic Test cases definition in #70
Generate API Readiness Checklist in #68
Changelog input
Additional documentation
This section can be blank.