<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [x] Commit message has the format required by CONTRIBUTING guide - [x] Commits are split per component (core, individual modules, libs, utils, ...) - [x] Each component has a single commit (if not, squash them into one commit) - [x] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [x] Small bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [x] PR should be backported to stable branches - [x] Tested changes locally - [x] Related to issue #2574
#### Description <!-- Describe your changes in detail -->
##### http_client: http_client_request shall include default clientcert, clientkey,... The lost module uses http_client API functions and in the course of NG112 client certificates are used for authentication when querying LIS or ECRF, the fix allows these to be read out via http_client module parameters.
##### lost: new features LoST redirect and NAPTR lookup (LIS, LoST) The extension of the module allows dynamic querying of LIS/HELD and LOST services via NAPTR lookup. In the case of LOST, a redirect response is evaluated. In case a lost_held_request (used to connect to a LIS) is passed with an empty string ("") for the connection parameter, then P-A-I or From header value hostnames are used for NAPTR lookup to get a corresponding service.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/2606
-- Commit Summary --
* initial commit
-- File Changes --
M src/modules/http_client/functions.c (12) M src/modules/lost/functions.c (575) M src/modules/lost/lost.c (5) A src/modules/lost/naptr.c (254) A src/modules/lost/naptr.h (38) M src/modules/lost/pidf.c (5) A src/modules/lost/response.c (946) A src/modules/lost/response.h (131) M src/modules/lost/utilities.c (229) M src/modules/lost/utilities.h (5)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/2606.patch https://github.com/kamailio/kamailio/pull/2606.diff
@wkampich - the git commit of this PR has the message `initial commit`, which clearly is not describing what it does. It also patches two modules.
You have to split the commit in two, one for the http_client module and one for the lost module, then for each commit the description message has to follow the format from the contributing guidelines:
* https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md#com...
@wkampich pushed 1 commit.
6f1e776f9eb83b256d73c223790b1de2b3df840d lost: new features LoST redirect and NAPTR lookup (LIS, LoST)
@wkampich pushed 1 commit.
6a896c9ba54d65b39be3b6d803730203ce299fe9 Delete kamctl
I have tested this PR on my local PC. During tests, I found module works as expected.
@wkampich - it seems that the PR is now removing the kamctl tool completely from the git repo. I guess you didn't want that.
At the end, you can push changes to lost module as you want, because you are the developer of the module. Of course, you can do PR whenever you want the review from other developers. Only for patches/commits to other modules/components is good to have PR.
@wkampich - it seems that the PR is now removing the kamctl tool completely from the git repo. I guess you didn't want that.
At the end, you can push changes to lost module as you want, because you are the developer of the module. Of course, you can do PR whenever you want the review from other developers. Only for patches/commits to other modules/components is good to have PR.
@miconda removing kamctl from the repo was not the intention ... sometimes it is better not to do things quickly on the side. The idea of the PR was just to get some feedback regarding #2606 before pushing changes to lost module. I would agree to remove the PR rather than merge it.
Closed #2606.
@wkampich regarding the code implementation - i looked also briefly into it. You took over some code from the enum module. If this is done copy and paste way, i.e. the same code in the enum module, we should probably think about moving this to the core. If you modified the code in different places, then probably the copyright notice of the original module needs to be added as well to your code.
Close this PR, as discussed in previous comments.
@henningw - if the reason of the PR is to get some feedback, why did you rush to close it? It no longer appears in the PR list for others to spot and comment. I guess @wkampich can close it when he finds it not being needed anymore, he has developer access to the project.
@wkampich - one clarification, maybe my first comment was confusing: there can be pull requests that change code in many modules, but it has to contain commits per module/component. In the first version of the PR, there was a part of the commit that changed http_client module along with the lost module.
I wanted to say that the PR has to contain two commits, one for http_client module and the other for the lost module. A PR is not restricted to a single commit. With the latest commit in this PR, I see that the part for http_client is no longer present.
He requested a review (in the git issue), which I did as well. It can be easily re-opened, do not wanted to rush something.
The comment about the role of PR was to get some feedback (https://github.com/kamailio/kamailio/pull/2606#issuecomment-768336527), I am expecting this means to discuss and get to some conclusions the developer is looking for and acknowledge the next steps.
I assume your `Close this PR, as discussed in previous comments` refers to his remark the he would agree to close the PR, but no real discussion was in between.
Reopened #2606.
@wkampich - one clarification, maybe my first comment was confusing: there can be pull requests that change code in many modules, but it has to contain commits per module/component. In the first version of the PR, there was a part of the commit that changed http_client module along with the lost module.
I wanted to say that the PR has to contain two commits, one for http_client module and the other for the lost module. A PR is not restricted to a single commit. With the latest commit in this PR, I see that the part for http_client is no longer present.
@wkampich - one clarification, maybe my first comment was confusing: there can be pull requests that change code in many modules, but it has to contain commits per module/component. In the first version of the PR, there was a part of the commit that changed http_client module along with the lost module. I wanted to say that the PR has to contain two commits, one for http_client module and the other for the lost module. A PR is not restricted to a single commit. With the latest commit in this PR, I see that the part for http_client is no longer present.
I created the commit in a hurry and unfortunately overlooked that lost and http_client are combined in one commit. My attempt to fix this quickly was not really successful. As I am currently extending the lost module for an ETSI interop. event in February, there will probably be other additions or fixes to the current lost module branch. Maybe it is better to close this PR and create a new one after the event to discuss the final version or get feedback from other developers. Then I will also consider the http_client commit.
OK.
Since the http_client change seemed to be a fix, I would suggest to make it a dedicated PR right now (well, when you get the time for it in the near future) and let the lost part for later, when you expect to finish the work planned for the event in February.
You can close this PR when you feel it should be done and you do not need anything else to be clarified.
@wkampich regarding the code implementation - i looked also briefly into it. You took over some code from the enum module. If this is done copy and paste way, i.e. the same code in the enum module, we should probably think about moving this to the core. If you modified the code in different places, then probably the copyright notice of the original module needs to be added as well to your code.
Close this PR, as discussed in previous comments.
@henningw There are indeed some functions copied from enum, perhaps it is good idea to have common NAPTR lookup utilities in the core.
OK.
Since the http_client change seemed to be a fix, I would suggest to make it a dedicated PR right now (well, when you get the time for it in the near future) and let the lost part for later, when you expect to finish the work planned for the event in February.
You can close this PR when you feel it should be done and you do not need anything else to be clarified.
OK. I'll do the http_client PR asap
Closed #2606.
-
Daniel-Constantin Mierla -
Henning Westerholt
-
sergey-safarov
-
Wolfgang Kampichler