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

V0.2.0 M4 #95

Closed
wants to merge 12 commits into from
Closed

Conversation

FabrizioMoggio
Copy link
Collaborator

@FabrizioMoggio FabrizioMoggio commented Aug 21, 2024

M4 version for Fall24:

What type of PR is this?

Add one of the following kinds:

  • cleanup
  • documentation

What this PR does / why we need it:

final release for Fall24

Which issue(s) this PR fixes:

#94
#93
#96
#97
#98
#100

Remove “-rc.n” from the version and server path: camaraproject#92
Remove "-rc.02" from "x-camara-commonalities": camaraproject#94
Missing "Bold" termination characters (**)
@hdamker
Copy link
Contributor

hdamker commented Aug 21, 2024

@FabrizioMoggio thanks for the release PR - have created camaraproject/ReleaseManagement#77 for the Release Management review.

Please update also the API Readiness checklist and the version within the .feature files as part of the PR.

@hdamker
Copy link
Contributor

hdamker commented Aug 21, 2024

@FabrizioMoggio and another important point:

The Release Notes for a public release must be in generell a summary of all changes since the last public release, not just the delta to the last release candidate.
In your case - as there was no previous public release and everything is new - it's more a list of "What's new" as you had in the first release candidate - you don't need to mention the corrections of this new stuff which happened between the release candidates, as for someone who is taking this public release, everything is anyway new.

Note: the "full changelog" link is
Full Changelog: https://github.com/camaraproject/CallForwardingSignal/commits/r1.3

bigludo7
bigludo7 previously approved these changes Aug 21, 2024
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
Your PR will be used as an example for me for other API Fabrizio :)

@bigludo7 bigludo7 self-requested a review August 21, 2024 15:45
@bigludo7
Copy link
Collaborator

Correction for my comment: this @FabrizioMoggio PR will be the example once the comment from @hdamker will be fixed ;)

Copy link
Contributor

@hdamker hdamker left a comment

Choose a reason for hiding this comment

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

  • Please update also the API Readiness checklist and the version within the .feature files as part of the PR
  • Complete the CHANGELOG for the public release with all updates, see my previous comment

@hdamker
Copy link
Contributor

hdamker commented Aug 23, 2024

BTW: release tracker updated ... r1.3 will be the public release tag, not the next pre-release.

@hdamker hdamker requested a review from a team August 27, 2024 15:38
@FabrizioMoggio FabrizioMoggio mentioned this pull request Aug 27, 2024
@FabrizioMoggio
Copy link
Collaborator Author

I’m sorry to ask for advice, I’m trying do to my best 😊 from my holidays. For M4 I should have addressed all your comments. The current PR for the CFS API is targeting the public release. What I’m not sure of is if at 30/08 the public release should be merged (if so, we missed it 😊) or if the final approval from @camaraproject/release-management_maintainers is due for M5 (09-15).

@hdamker
Copy link
Contributor

hdamker commented Aug 31, 2024

@FabrizioMoggio thank for taking car during your vacation! No worries, you have done every necessary for the M4 milestone by creating and updating this release PR so far. Indeed, the review and approval by Release Management will happen before M5 and then the merge.

@bigludo7 @chinaunicomyangfan Please review the PR and the latest changes as well and approve the PR already.

bigludo7
bigludo7 previously approved these changes Sep 2, 2024
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

Copy link
Collaborator Author

@FabrizioMoggio FabrizioMoggio left a comment

Choose a reason for hiding this comment

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

the requested changes by @hdamker have been implemented

@FabrizioMoggio
Copy link
Collaborator Author

  • Please update also the API Readiness checklist and the version within the .feature files as part of the PR
  • Complete the CHANGELOG for the public release with all updates, see my previous comment

Dear @hdamker I don't know how to move forward. You requested a change and I have implemented it but I can't find a way to mark the requested changes as implemented. So I can't merge. Is it you that need to control and mark them as implemented?

including: the changes from: camaraproject#99

implementing issue: camaraproject#100
Copy link

linux-foundation-easycla bot commented Sep 4, 2024

CLA Not Signed

bigludo7
bigludo7 previously approved these changes Sep 4, 2024
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

@FabrizioMoggio FabrizioMoggio dismissed bigludo7’s stale review September 4, 2024 11:01

The merge-base changed after approval.

Copy link
Contributor

@tanjadegroot tanjadegroot left a comment

Choose a reason for hiding this comment

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

a few broken links and typos to fix and this is OK: LGTM from release mgmt

This is the first official release of the Call Forwarding Signal API, version 0.2.0. It contains mainly alignments with the Commonalities 0.4.0 and the Release Management Guidelines.

- API definition **with inline documentation**:
- OpenAPI [YAML spec file](https://github.com/camaraproject/CallForwardingSignal/blob/r1.3/code/API_definitions/Call_Forwarding_Signal.yaml)
Copy link
Contributor

Choose a reason for hiding this comment

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

All 3 links for API display in this section need to be updated to point to the new file name f the API yaml

Copy link
Contributor

Choose a reason for hiding this comment

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

Line 19 (location: virtually) can be removed from teh scope section.

- OpenAPI [YAML spec file](https://github.com/camaraproject/CallForwardingSignal/blob/r1.2/code/API_definitions/Call_Forwarding_Signal.yaml)
- [View it on ReDoc](https://redocly.github.io/redoc/?url=https://raw.githubusercontent.com/camaraproject/CallForwardingSignal/r1.2/code/API_definitions/Call_Forwarding_Signal.yaml&nocors)
- [View it on Swagger Editor](https://editor.swagger.io/?url=https://raw.githubusercontent.com/camaraproject/CallForwardingSignal/r1.2/code/API_definitions/Call_Forwarding_Signal.yaml)
- OpenAPI [YAML spec file](https://github.com/camaraproject/CallForwardingSignal/blob/r1.3/code/API_definitions/Call_Forwarding_Signal.yaml)
Copy link
Contributor

Choose a reason for hiding this comment

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

All 3 links here need to be updated with the new API file name

Copy link
Contributor

Choose a reason for hiding this comment

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

Just 2 clarification questions

  • on line 229: does unconditional forwarding overwrite conditional forwarding if both are active ?
  • on line 230: with "This endpoit exceeds the main scope of the CFS API, for this reason an error code 501 can be returned" do you mean that this second endpoint is optional and may not be provided and that 501 is returned in that case ?
  • there is a type in "endpoint" in line 229

Copy link
Contributor

Choose a reason for hiding this comment

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

Line 132: typo: involked -> invoked
Line 173: typo requst -> request

@@ -1,21 +1,21 @@

# API Readiness Checklist

Checklist for API call-forwarding-signal v0.2.0-rc.2 in r1.2
Checklist for API call-forwarding-signal v0.2.0 in r1.3


| Nr | API release assets | alpha | release-candidate | public-release<br>initial | public-release<br> stable | Status | Comments |
|----|----------------------------------------------|:-----:|:-----------------:|:-------:|:------:|:----:|:----|
| 1 | API definition | M | M | M | M | Y | /code/API_definitions/Call_Forwarding_Signal.yaml |
Copy link
Contributor

Choose a reason for hiding this comment

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

change to new API yaml file name (call-forwarding-signal.yaml)

@FabrizioMoggio
Copy link
Collaborator Author

Closed because EasyCLA problems.

Replaced by: #101

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation Fall24
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants