apisix-build-tools icon indicating copy to clipboard operation
apisix-build-tools copied to clipboard

Published 20 hours ago • verified publisher icon api7

improvement: deprecate openldap dependency for apisix

Open Zhenye-Na opened this issue 2 years ago • 15 comments

This PR is in the scope of this issue in apisix repo: https://github.com/apache/apisix/issues/7865

Zhenye-Na avatar Oct 08 '23 03:10 Zhenye-Na

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Oct 08 '23 03:10 CLAassistant

Please make the ci pass

monkeyDluffy6017 avatar Oct 08 '23 03:10 monkeyDluffy6017

Please make the ci pass

Please correct me if I am wrong but it seems like a circular dependency happened between https://github.com/api7/apisix-build-tools/pull/337 and https://github.com/apache/apisix/pull/10176

  1. This PR pulls latest of apisix where openldap is in the rockspec
  2. https://github.com/apache/apisix/pull/10176 depends on the build tool change in this PR

Zhenye-Na avatar Oct 08 '23 03:10 Zhenye-Na

@Zhenye-Na Please fix the CI errors first.

kingluo avatar Oct 08 '23 07:10 kingluo

As I mentioend above, the circular dependency of this repo and apache/apisix occurs here: https://github.com/api7/apisix-build-tools/actions/runs/6445185009/workflow?pr=337#L44

this package is building apisix with the master branch, where lualdap/openldap exists. However, this PR delted the dependency which caused the issue.

If it is ok to you @kingluo , I could redirect the line above to my own forked branch to see if CI could pass.

Zhenye-Na avatar Oct 09 '23 05:10 Zhenye-Na

@Zhenye-Na ok, if you confirm these 4 errors are expected due to circular dependencies (I think so after reading the code), I'll approve it. @monkeyDluffy6017 @membphis @moonming What about your opinions?

kingluo avatar Oct 09 '23 11:10 kingluo

@Zhenye-Na ok, if you confirm these 4 errors are expected due to circular dependencies (I think so after reading the code), I'll approve it. @monkeyDluffy6017 @membphis @moonming What about your opinions?

Please let me know what else I could test to make sure there is nothing I missed regarding this dependency deprecation. Meanwhile, if there is a way to inject my personal fork to test things out here, that will be even better.

Thanks!

Zhenye-Na avatar Oct 09 '23 19:10 Zhenye-Na

@Zhenye-Na Is this PR prerequisite for https://github.com/apache/apisix/pull/10176?

Revolyssup avatar Mar 07 '24 05:03 Revolyssup

@Zhenye-Na Can you fix the merge conflicts?

Revolyssup avatar Mar 07 '24 05:03 Revolyssup

@Zhenye-Na Is this PR prerequisite for apache/apisix#10176?

Yes

Zhenye-Na avatar Mar 13 '24 04:03 Zhenye-Na

@Zhenye-Na Can you fix the merge conflicts?

I could work on this, but will definitely appreciate any guidance of merging this PR later

Zhenye-Na avatar Mar 13 '24 04:03 Zhenye-Na

@Zhenye-Na Can you fix the merge conflicts?

I could work on this, but will definitely appreciate any guidance of merging this PR later

When can you work on this?

Revolyssup avatar Mar 13 '24 17:03 Revolyssup

@Zhenye-Na Can you fix the merge conflicts?

I could work on this, but will definitely appreciate any guidance of merging this PR later

When can you work on this?

Please allow me to rebase and resolve any merge conflicts by end of this Friday.

I could ping you here once I got next revision out, if it works for you ?

thanks

Zhenye-Na avatar Mar 14 '24 18:03 Zhenye-Na

Missed the committed SLA, will prioritize this patch during this work week.

Thanks

Zhenye-Na avatar Mar 18 '24 19:03 Zhenye-Na

@Revolyssup just resolved merge conflicts

Zhenye-Na avatar Mar 19 '24 02:03 Zhenye-Na