fix: remove openldap dependencies from apisix #10176
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
monkeyDluffy6017
added
the
wait for update
|
Looks like some files are ignored. # cd to apisix repo dir
git grep -i ldapNot all files need to remove ldap, .e.g. t/ldap-autht.t. |
Ack. I will create a dedicated CR to address the following comments
Does this sound good to you ? @monkeyDluffy6017 Also, do you prefer if I create a new Issue regarding this change so that the CR will only fix that Issue instead of the current one ? Thanks |
|
Hi @kingluo , Regarding the deletion of the documentation page, could you help me confirm that the following sections could be removed safely? Thanks!
If I could safely remove those references, I guess it also applies to the other documentation page as well ? For example:
|
|
@Zhenye-Na What do you mean by |
|
@Zhenye-Na Yes, you should look over all LDAP-related lines from the grep result and everything related to OpenLDAP (not LDAP) should be removed. Anyway, I'll review your output to ensure correctness if your PR passes the CI. |
Sorry, I was referring to |
Thank you! |
|
Converting this PR to be in |
|
Hi @kingluo , I have executed the following commands to retrieve the list of files with This only outputs the filenames: git grep --files-with-matches -i ldap
CHANGELOG.md
apisix/plugins/ldap-auth.lua
ci/pod/openfunction/function-example/test-uri/go.sum
ci/redhat-ci.sh
conf/config-default.yaml
docs/en/latest/config.json
docs/en/latest/getting-started/key-authentication.md
docs/en/latest/plugins/ldap-auth.md
docs/en/latest/plugins/wolf-rbac.md
docs/en/latest/stream-proxy.md
docs/en/latest/tutorials/add-multiple-api-versions.md
docs/zh/latest/CHANGELOG.md
docs/zh/latest/config.json
docs/zh/latest/plugins/ldap-auth.md
docs/zh/latest/plugins/wolf-rbac.md
docs/zh/latest/stream-proxy.md
rockspec/apisix-2.11.0-0.rockspec
rockspec/apisix-2.12.0-0.rockspec
rockspec/apisix-2.12.1-0.rockspec
rockspec/apisix-2.13.0-0.rockspec
rockspec/apisix-2.13.1-0.rockspec
rockspec/apisix-2.13.2-0.rockspec
rockspec/apisix-2.13.3-0.rockspec
rockspec/apisix-2.14.0-0.rockspec
rockspec/apisix-2.14.1-0.rockspec
rockspec/apisix-2.15.0-0.rockspec
rockspec/apisix-2.15.1-0.rockspec
rockspec/apisix-2.15.2-0.rockspec
rockspec/apisix-2.15.3-0.rockspec
rockspec/apisix-2.99.0-0.rockspec
rockspec/apisix-3.0.0-0.rockspec
rockspec/apisix-3.1.0-0.rockspec
rockspec/apisix-3.2.0-0.rockspec
rockspec/apisix-3.2.1-0.rockspec
rockspec/apisix-3.3.0-0.rockspec
rockspec/apisix-3.4.0-0.rockspec
rockspec/apisix-3.5.0-0.rockspec
rockspec/apisix-master-0.rockspec
t/admin/plugins.t
t/chaos/utils/Dockerfile
t/plugin/ldap-auth.tHere is the full list of filename + references I think you mentioned above that I have made additional changes like Thanks! |
Zhenye-Na
force-pushed
the
remove-openldap
branch
from
4308d1b to
5cf1be3
Compare
|
@Zhenye-Na Please rerun git grep and recheck the results when you're done making changes, i.e. no openldap-related files anymore, then I'll run CI again. |
|
Here are the outputs after I re-run |
|
@Zhenye-Na Please check the CI errors. |
|
Acknowledged. Meanwhile, I am wondering is there a way that I could initiated the CI testing on my own to reduce the communication back and forth? Or perhaps, there is a way to test the codes locally and I didn't come across that page, could you help point me to it ? Thanks! |
fork the repo, create a branch there to make code changes, and raise PR to the master branch of your fork. |
|
I was checking the errors of the CI Builds It looks like one of the dependencies Appreciate it if there are any pointers to invesitgate more on this issue. |
Zhenye-Na
force-pushed
the
remove-openldap
branch
from
c82b6f2 to
1c0281b
Compare
I understood. please see comment in api7/apisix-build-tools#337 (comment), before I push out the fix for this |
kingluo
requested changes
Oct 9, 2023
ci/common.sh
Outdated
| @@ -20,6 +20,7 @@ set -ex | |||
| export_or_prefix() { | |||
| export OPENRESTY_PREFIX="/usr/local/openresty-debug" | |||
| export APISIX_MAIN="https://raw.githubusercontent.com/apache/incubator-apisix/master/rockspec/apisix-master-0.rockspec" | |||
| export APISIX_LDAP_DEPRECATION="https://raw.githubusercontent.com/Zhenye-Na/apisix/remove-openldap/rockspec/apisix-master-0.rockspec" | |||
There was a problem hiding this comment.
Please remove the personal link and use the local file apisix-master-0.rockspec.
There was a problem hiding this comment.
If I remove this reference to use the apache/apisix master branch, lualdap will be included during the dependencies installation. This will cause the issue happened before, which is lualdap.h is missing the configuration
There was a problem hiding this comment.
Though I am not too sure regarding the ldap-auth testing failures as I was not touching any files that are referencing the dependencies of ldap-auth. Maybe I am missing something ?
| @@ -39,7 +39,7 @@ script() { | |||
| cp -r ../utils ./ | |||
|
|
|||
| # install APISIX by luarocks | |||
| luarocks install $APISIX_MAIN > build.log 2>&1 || (cat build.log && exit 1) | |||
| luarocks install $APISIX_LDAP_DEPRECATION > build.log 2>&1 || (cat build.log && exit 1) | |||
There was a problem hiding this comment.
ack, will be replacing the reference of my own branch to mainline, once we are ready to merge this PR
|
@Zhenye-Na What else does this pr need in order to be merged? |
From my point of view, this PR and api7/apisix-build-tools#337 need to be merged together as you mentioned the following in this PR comment above ^
|
|
@kingluo Could you help check if there is any remaining work pending? |
|
@Zhenye-Na merge api7/apisix-build-tools#337 first, and the ci here will pass, right? |
Hey @kingluo , sorry for the late reply here. To be honest the CI test cases failed in this PR, I could see that most of them is for I did not change anything related to As long as api7/apisix-build-tools#337 makes sense to you, we could merge that PR first and either you or me could re-trigger the CI testing to verify |
|
@Zhenye-Na I will continue to review this pr, Thanks! |
No worries, let's work together to sort this out. Thanks |
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions. |
|
@Zhenye-Na are you going to work on this? |
|
@Zhenye-Na Do you need this api7/apisix-build-tools#337 PR merged in order to go forward with this PR? |
Hello @Revolyssup , If I remember this clearly, hese two PRs are circular dependent on each other. So I believe the PR to build-tool package is better to be merged first |
|
Makes sense |
Zhenye-Na
force-pushed
the
remove-openldap
branch
from
4997bce to
c743fb1
Compare
Zhenye-Na
force-pushed
the
remove-openldap
branch
from
c743fb1 to
7933843
Compare
|
I just cleaned up the merge conflicts here as well, thanks! |
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions. |
|
This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #7865
Checklist
Modified files checklist based on original issue:
openldapor notutils/linux-install-openresty.shcould not be found