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.

Sign up for GitHub

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

Jump to bottom

feat: remove rust dependency by rollback lua-resty-ldap on master #9936

Merged
merged 5 commits into from
Aug 21, 2023
Merged

feat: remove rust dependency by rollback lua-resty-ldap on master #9936

merged 5 commits into from
Aug 21, 2023

Conversation

Revolyssup
Copy link
Contributor

@Revolyssup Revolyssup commented Jul 31, 2023 •
edited
Loading

Description

Fixes #9891

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@shreemaan-abhishek
Copy link
Contributor

@Revolyssup, eventually in the future we might have to upgrade lua-resty-ldap version again. This doesn't make sense to me as a good solution in the long run.

@monkeyDluffy6017
Copy link
Contributor

@shreemaan-abhishek What's your opinion?

@monkeyDluffy6017
Copy link
Contributor

@Revolyssup please fix the ci

@monkeyDluffy6017 monkeyDluffy6017 added the wait for update wait for the author's response in this issue/PR label Aug 3, 2023
@shreemaan-abhishek
Copy link
Contributor

shreemaan-abhishek commented Aug 3, 2023 •
edited
Loading

@monkeyDluffy6017, right now everything is fine CI works well why do we need to remove rust dependency in the first place? My opinion is to let things be how everything is right now.

cc: @Sn0rt

@Revolyssup
fix merge conflicts
b712eb9 
Signed-off-by: revolyssup <[email protected]>
@Revolyssup
fix comma in rockspec
df2a043 
Signed-off-by: revolyssup <[email protected]>
@Revolyssup
Copy link
Contributor Author

Revolyssup commented Aug 3, 2023 •
edited
Loading

ldap
@Sn0rt @monkeyDluffy6017 As you can see, a lot of features/fixes have been added after 0.1.0. And most importantly the client module was abstracted so the API changed a little, which means the code would need to be refactored as per the older version. The older version didn't have client.lua. Simple rolling back the version wouldn't work and refactoring code would mean the code on master can either be compatible with 0.1.0 or the newer version.
What should I do here?

@Revolyssup
Copy link
Contributor Author

To be moved forward after this comment is addressed. #9891 (comment)

@Revolyssup
solve merge conflicts
f0cdec3 
Signed-off-by: Ashish Tiwari <[email protected]>
@Revolyssup
refactor apisix for ldap0.1
33bb96b 
Signed-off-by: Ashish Tiwari <[email protected]>
@Revolyssup
Copy link
Contributor Author

Current approach: lua-resty-ldap is moved back to v0.1.0 and apisix has been refactored accordingly.

@Revolyssup Revolyssup removed the wait for update wait for the author's response in this issue/PR label Aug 16, 2023
@shreemaan-abhishek shreemaan-abhishek merged commit ac3992f into apache:master Aug 21, 2023
32 checks passed
hongbinhsu pushed a commit to fitphp/apix that referenced this pull request Aug 22, 2023
Merge branch 'upstream/master' into github/master
aa6b2b5 
* upstream/master: (77 commits)
  docs: Update admin-api.md (apache#10056)
  ci: fix a bug that can not open nginx.pid (apache#10061)
  feat: remove rust dependency by rollback lua-resty-ldap on master (apache#9936)
  docs: fix grpc-transcode.md error (apache#10059)
  feat: upgrade lua dependencies (apache#10051)
  fix: rollback lua-resty-session to 3.10 (apache#10046)
  feat: upgrade resty-redis-cluster from  1.02-4->1.05-1 (apache#10041)
  feat: update lua library (apache#10037)
  fix: worker not exited when executing quit or reload command (apache#9909)
  fix: traffic split plugin not validating upstream_id (apache#10008)
  ci: update the timeout value in cli.yml (apache#10026)
  fix(tencent-cloud-cls): DNS parsing failure (apache#9843)
  chore(deps): bump actions/setup-node from 3.7.0 to 3.8.0 (apache#10025)
  feat(openid-connect): add proxy_opts attribute (apache#9948)
  perf(log-rotate): replace string.sub with string.byte (apache#9984)
  fix(ci): replace github action in update-labels.yml (apache#9987)
  fix: can't sync etcd data if key has special character (apache#9967)
  perf(aws-lambda): cache the index of the array (apache#9944)
  fix: add support for dependency installation on endeavouros (apache#9985)
  chore(ci): automate management of unresponded issues (apache#9927)
  ...
@Revolyssup Revolyssup mentioned this pull request Aug 31, 2023
Merged
5 tasks
@FOWind FOWind mentioned this pull request Dec 25, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlinsRan AlinsRan AlinsRan approved these changes

@moonming moonming moonming approved these changes

@Sn0rt Sn0rt Sn0rt approved these changes

@bzp2010 bzp2010 Awaiting requested review from bzp2010

@monkeyDluffy6017 monkeyDluffy6017 Awaiting requested review from monkeyDluffy6017

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: As a user, I want to remove rust dependencies, so that reconsider how to install ldap dependencies
6 participants
@Revolyssup @shreemaan-abhishek @monkeyDluffy6017 @Sn0rt @moonming @AlinsRan