Skip to content

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

Merged
shreemaan-abhishek merged 5 commits into
apache:masterfrom
Revolyssup:revolyssup/9891
Aug 21, 2023
Merged

feat: remove rust dependency by rollback lua-resty-ldap on master#9936
shreemaan-abhishek merged 5 commits into
apache:masterfrom
Revolyssup:revolyssup/9891

Conversation

@Revolyssup

@Revolyssup Revolyssup commented Jul 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

shreemaan-abhishek commented Aug 2, 2023

Copy link
Copy Markdown
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.

@juststillthinking

juststillthinking commented Aug 3, 2023

Copy link
Copy Markdown
Contributor

@shreemaan-abhishek What's your opinion?

@juststillthinking

juststillthinking commented Aug 3, 2023

Copy link
Copy Markdown
Contributor

@Revolyssup please fix the ci

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

shreemaan-abhishek commented Aug 3, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

@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

Revolyssup commented Aug 3, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

Revolyssup commented Aug 10, 2023

Copy link
Copy Markdown
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

Revolyssup commented Aug 16, 2023

Copy link
Copy Markdown
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
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
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@moonming moonming moonming approved these changes
@AlinsRan AlinsRan AlinsRan approved these changes
@bzp2010 bzp2010 Awaiting requested review from bzp2010
@juststillthinking juststillthinking Awaiting requested review from juststillthinking
+1 more reviewer
@Sn0rt Sn0rt Sn0rt approved these changes
Reviewers whose approvals may not affect merge requirements

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 @juststillthinking @Sn0rt @moonming @AlinsRan