Add OpenLDAPSyncreplCookie class #571
Conversation
mistotebe
force-pushed
the
OpenLDAPSyncreplCookie
branch
3 times, most recently
from
beb1513 to
3247211
Compare
droideck
left a comment
droideck
left a comment
There was a problem hiding this comment.
A few minor comments, but the code looks good!
We need to figure out, though, what's wrong with the CI... I can't even restart it.
mistotebe
force-pushed
the
OpenLDAPSyncreplCookie
branch
3 times, most recently
from
6a15068 to
672779f
Compare
droideck
left a comment
droideck
left a comment
There was a problem hiding this comment.
LGTM!
Could you please try to rebase and see if tests pass?
mistotebe
force-pushed
the
OpenLDAPSyncreplCookie
branch
from
672779f to
2458039
Compare
mistotebe
force-pushed
the
OpenLDAPSyncreplCookie
branch
from
2458039 to
71952ad
Compare
Just rebasing didn't seem to work, but given that @spaceone had some comments and I forgot to register the tests, this rebase seems to have passed made pypy310 happy. We might still be closing a fd twice somewhere else in the code? |
droideck
left a comment
droideck
left a comment
There was a problem hiding this comment.
Very minor comments, otherwise, LGTM!
Tests/t_ldap_syncrepl.py
Outdated
|
|
||
| # An active MPR should not have a sid=000 server in it | ||
| if self.server.server_id == 0: | ||
| self.skip("Server got serverid 0 assigned") |
There was a problem hiding this comment.
Looking at these methods - https://docs.python.org/3/library/unittest.html#unittest.TestCase , I think it should be:
self.skipTest("Server got serverid 0 assigned")the only place I see .skip() - is as a decorator.
Lib/ldap/syncrepl.py
Outdated
| pass | ||
| return result | ||
|
|
||
| def update(self, cookie: str): |
There was a problem hiding this comment.
The signature says cookie: str but _parse_cookie() accepts AnyStr (both str and bytes). Worth make it consistent.
| pass | ||
|
|
||
|
|
||
| class OpenLDAPSyncreplCookie: |
There was a problem hiding this comment.
Should we also export the class in __all__?
mistotebe
force-pushed
the
OpenLDAPSyncreplCookie
branch
from
71952ad to
e966aaa
Compare
|
Thanks @droideck for spotting those, all of them should have been there in the first place. Sorry about that, should be fixed now. |
3 participants
No description provided.