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.

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

refactor: Switch from ldap_poller to a direct implementation #106

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

tlater-famedly
Copy link
Contributor

@tlater-famedly tlater-famedly commented Jan 20, 2025

This cuts out an incredible amount of middle-man code that is completely unnecessary for famedly-sync in practice.

Also cleans up a couple of inherited bugs (like not correctly quitting if we run into a connection issue), and hopefully increases logging capacity. Basically, a prerequisite of famedly/ldap-poller#36, since this lets us reasonably fix the issue downstream.

TODO:

  • Rewrite the currently sync bits with async code
    • Apparently ldap3 created a sync interface by just calling block_on-type things on their async methods, so it is completely incompatible with otherwise async code
  • Check if this actually gives us more logs
    • Probably not actually necessary to merge this, it's good-to-have anyway
  • Figure out what the deleted entries thing is about again
  • Get the tests to pass
  • Write some new tests to assert the new bits work well
  • Cover error cases

@tlater-famedly tlater-famedly requested a review from a team as a code owner January 20, 2025 12:30
@tlater-famedly tlater-famedly marked this pull request as draft January 20, 2025 12:31
@tlater-famedly tlater-famedly force-pushed the tlater/fix-ldap-sync branch 3 times, most recently from b5dea38 to 0ef3070 Compare January 21, 2025 03:51
Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 99.12281% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.86%. Comparing base (5ab1845) to head (727c4d9).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/sources/ldap.rs 99.12% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #106      +/-   ##
==========================================
- Coverage   93.18%   92.86%   -0.32%     
==========================================
  Files           9        9              
  Lines        1716     1669      -47     
==========================================
- Hits         1599     1550      -49     
- Misses        117      119       +2     
Files with missing lines Coverage Δ
src/sources/ldap.rs 96.79% <99.12%> (-1.07%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ab1845...727c4d9. Read the comment docs.

@tlater-famedly tlater-famedly force-pushed the tlater/fix-ldap-sync branch 6 times, most recently from efd7c49 to 4a49353 Compare January 21, 2025 04:59
@tlater-famedly tlater-famedly marked this pull request as ready for review January 21, 2025 05:03
@tlater-famedly tlater-famedly force-pushed the tlater/fix-ldap-sync branch 2 times, most recently from c9e4660 to 6f2e7c6 Compare January 21, 2025 05:43
@tlater-famedly tlater-famedly merged commit 727c4d9 into main Jan 22, 2025
5 checks passed
@tlater-famedly tlater-famedly deleted the tlater/fix-ldap-sync branch January 22, 2025 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants