search

389ds / 389-ds-base

The enterprise-class Open Source LDAP server for Linux
https://www.port389.org/
Other
211 stars 91 forks source link

PR - Added python3 support and code fixes. [WIP] #3346

Closed 389-ds-bot closed 4 years ago

389-ds-bot commented 4 years ago

Cloned from Pagure Pull-Request: https://pagure.io/389-ds-base/pull-request/50287

Description: Added py3 support by explicitly changing strings to bytes. Codes are fixed, which are stated in each commit.

Resolves: #2647

Reviewed by: ??

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-03-19 01:33:09

Don't duplicate this, use strcmdline = " ".join(cmdline) instead.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-03-19 01:33:30

Sleep? Is there a better way?

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-03-19 01:34:13

Probably means you have a candidate for rewriting this modify to use the Plugin(DSldapObjects) api ....

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-03-19 01:34:19

And here.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-03-19 01:34:26

And here.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-03-19 01:34:56

Isn't there a "replication manager" dsldapobject you can use instead? IIRC I made one for the replication code a few years back.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-03-19 01:36:26

In general, if you are going to port from py2 -> py3, that doesn't mean slap bytes on it, it means rewrite parts that need dsldapobjects instead.

I also would actually propose removing that crypto/tls cipher count test. It breaks every verison, it proves nothing, and generally is annoying to maintain in any capacity. I'd rather just remove it. @droideck may have a comment on that though ....

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-03-19 06:52:23

Isn't there a "replication manager" dsldapobject you can use instead? IIRC I made one for the replication code a few years back.

It is, but in the test, replication is actually working but giving an invalid credential error for replication manager. I could not access the service accounts, so I have created another manager.

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-03-19 07:49:48

In general, if you are going to port from py2 -> py3, that doesn't mean slap bytes on it, it means rewrite parts that need dsldapobjects instead. I also would actually propose removing that crypto/tls cipher count test. It breaks every verison, it proves nothing, and generally is annoying to maintain in any capacity. I'd rather just remove it. +1 @droideck may have a comment on that though...

I agree, py2 -> py3 is not just adding bytes, but there are a lot of failures which needs to be fixed first so that build quality could be analyzed first. there are a lot of tests which are failing and we are unable to have a proper say on the builds. We actually wanna avoid it as much we can, anyways I will still see we can quickly remove this :)

389-ds-bot commented 4 years ago

Comment from mhonek (@kenoh) at 2019-03-19 10:36:40

The associated commit message is entirely wrong.

EDIT: I meant the other one (3c7d46224 for dirsrvtests/tests/tickets/ticket48194_test.py).

389-ds-bot commented 4 years ago

Comment from mhonek (@kenoh) at 2019-03-19 10:40:53

Please explain what and why this fixes. What's the actual error? This really needs a comprehensive commit message.

389-ds-bot commented 4 years ago

Comment from mhonek (@kenoh) at 2019-03-19 10:44:32

Why this hardcoded value? Where does it come from? Cannot it be pulled in from anywhere?

389-ds-bot commented 4 years ago

Comment from mhonek (@kenoh) at 2019-03-19 10:57:51

AFAICT this file's change effectively removes a test for a feature from cab38f97 introduced just a few weeks ago. What is the story here?

389-ds-bot commented 4 years ago

Comment from mhonek (@kenoh) at 2019-03-19 10:59:35

This was introduced 3 weeks ago - why is this wrong? (not saying it is not, just need an explanation)

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-03-19 14:02:46

rebased onto 4737790b289136ab3d3e65dfa04626f09b28815d

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-03-19 14:41:23

1 new commit added

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-03-19 14:57:51

5 new commits added

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-03-19 15:05:54

5 new commits added

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-03-19 15:50:06

5 new commits added

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-03-19 16:10:50

rebased onto 024aa5b302ffa054f65dc54520040f27517514fb

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-03-19 16:15:28

This was introduced 3 weeks ago - why is this wrong? (not saying it is not, just need an explanation)

Was having some merge conflict issue, resolved and fixed. also, meaningful commit message is been added.

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-03-19 17:27:39

This was introduced 3 weeks ago - why is this wrong? (not saying it is not, just need an explanation)

Was having some merge conflict issue, resolved and fixed. also, meaningful commit message is been added.

@kenoh But I see them passing in the latest builds will remove it from the commit.

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-03-19 18:07:34

rebased onto 8b7328b3d9947db82e8cbdba63cb1dd839c4a832

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-03-19 18:16:58

Please explain what and why this fixes. What's the actual error? This really needs a comprehensive commit message.

I will paste a complete output of the error, one was the cn=RSA entry which I removed. Will update with the error if the changes in the code are not done.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-03-20 01:08:32

I think the openssl test is pretty bad anyway, like ... we are testing something that is really dynamic and fragile, and a bit, not our problem?

389-ds-bot commented 4 years ago

Comment from mhonek (@kenoh) at 2019-03-20 01:54:12

@aadhikari Out of curiosity, what is the build you're testing?

In general, tests should definitely pass on master. If they don't pass on older builds then relevant fixes are welcome of course, but only those that don't break the tests on master. E.g. with pwp_history_test.py here, (it should have been done so before, but we can do it now, too) we can properly make the test conditional using ds_is_older or ds_is_newer so that it passes on versions without the new feature and those with it, too.

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-03-20 08:32:50

@aadhikari Out of curiosity, what is the build you're testing? In general, tests should definitely pass on master. If they don't pass on older builds then relevant fixes are welcome of course, but only those that don't break the tests on master. E.g. with pwp_history_test.py here, (it should have been done so before, but we can do it now, too) we can properly make the test conditional using ds_is_older or ds_is_newer so that it passes on versions without the new feature and those with it, too.

@kenoh I was testing on 389-ds-base: 1.4.0.21-1.fc29 and 389-ds-base: 1.4.1.1-20190320gitf16615485.fc29.

Also what I observed is the timing of the sleep. The same code may not Rework for the first time but can work for the second time.

Just for the clarification: Result for build: 1.4.0.21-1.fc29 - https://paste.fedoraproject.org/paste/Xg3NZ61ilVgI0aT7Eej~iw Result for build: 389-ds-base: 1.4.1.1-20190320gitf16615485.fc29 - https://paste.fedoraproject.org/paste/AJ~65WXImPX5ZDWZVeIqLw Result form latest build: https://paste.fedoraproject.org/paste/KXRROyTs66LCS62hiHNBeg

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-03-20 11:20:23

So, form the result what I can get is we need to change the sleep to a line where the set of operation which is causing failures should be done then sleep should be used. Secondly, I am guessing, 0 as a minimum range value for passwordInHistory was acceptable before, we changed that in between and now it is back to the default range?

389-ds-bot commented 4 years ago

Comment from mhonek (@kenoh) at 2019-03-20 17:18:53

@aadhikari Not sure why not for you but the test passes for me even in a loop. I've tested with 1.4.1.1-20190320git33fbced25.fc28. I don't see a point in testing the 1.4.1.1-20190320gitf16615485.fc29 which is fairly old.

Looking at https://paste.fedoraproject.org/paste/KXRROyTs66LCS62hiHNBeg at the following (notice missing password1 there):

pwp_history_test.py        132 ERROR    password history: [b'20190320101020Zpassword', b'20190320101020Zpassword2', b'20190320101020Zpassword3']

it seems like user.set('userpassword', 'password1') does not set the new password correctly, which is why the test fails for you. I cannot reproduce, so please try to investigate why it happens for you.

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-03-25 10:07:42

@aadhikari Not sure why not for you but the test passes for me even in a loop. I've tested with 1.4.1.1-20190320git33fbced25.fc28. I don't see a point in testing the 1.4.1.1-20190320gitf16615485.fc29 which is fairly old. Looking at https://paste.fedoraproject.org/paste/KXRROyTs66LCS62hiHNBeg at the following (notice missing password1 there): pwp_history_test.py 132 ERROR password history: [b'20190320101020Zpassword', b'20190320101020Zpassword2', b'20190320101020Zpassword3']

it seems like user.set('userpassword', 'password1') does not set the new password correctly, which is why the test fails for you. I cannot reproduce, so please try to investigate why it happens for you.

The issue is not reproducible every time, will investigate.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-03-27 00:13:17

Highly likely if you are seeing an odd behaviour, it's a race condidition between replication and the bind or similar, so I'd check you are using ReplicationManager's "wait_for_repl" instead of sleep. That's my first instinct here ....

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2019-04-12 13:01:29

Is there any update? Do you have any blocker that we can help you with?

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-05-30 15:56:57

rebased onto 37c3e8a558aa638a298dc083eb5913e78d5bb947

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-05-30 17:19:30

rebased onto eb71a079621f53948561f5a8b6b6c0964e114717

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-05-30 17:41:59

rebased onto 35ee0be0e8062978f01648bdd6b0e28e28f8f06a

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-05-31 12:47:14

rebased onto b7ba0f2562c9b2ead6d9247e5a4dd40df7a9d323

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-05-31 12:49:20

3 new commits added

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-05-31 13:42:09

3 new commits added

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-05-31 14:03:25

rebased onto 3dc678e95b291adf2b38194c46eb7bac3e9aca28

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-05-31 14:14:47

Isn't there a "replication manager" dsldapobject you can use instead? IIRC I made one for the replication code a few years back.

@Firstyear I tried using topology_m2.ms["master2"].replica.create_repl_manager() by passing/NOT passing password and dn, got this error: ERR - slapi_ldap_bind - Error: could not bind id [cn=replication manager,cn=config] authentication mechanism [SIMPLE]: error 49 (Invalid credentials)

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-05-31 14:18:23

@Firstyear Rest all the changes have been made, also @droideck please let us know what we should be doing with ticket47838_test.py, the ciphers are very dynamic as mentioned by @Firstyear Thanks!

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-06-03 10:06:59

Isn't there a "replication manager" dsldapobject you can use instead? IIRC I made one for the replication code a few years back.

@Firstyear I tried using topology_m2.ms["master2"].replica.create_repl_manager() by passing/NOT passing password and dn, got this error: ERR - slapi_ldap_bind - Error: could not bind id [cn=replication manager,cn=config] authentication mechanism [SIMPLE]: error 49 (Invalid credentials)

Look at:

https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/cli_conf/replication.py#_178

This is how you use the "BootstrapReplicationManager" to create the replication manager dn's

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-06-03 10:08:28

@Firstyear Rest all the changes have been made, also @droideck please let us know what we should be doing with ticket47838_test.py, the ciphers are very dynamic as mentioned by @Firstyear Thanks!

rm ./dirsrvtests/tests/tickets/ticket47838_test.py

I never want to see it again, it's a silly test. I think it adds no value and no help.

@droideck may have a different view though :)

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2019-06-03 12:01:41

@Firstyear Rest all the changes have been made, also @droideck please let us know what we should be doing with ticket47838_test.py, the ciphers are very dynamic as mentioned by @Firstyear Thanks!

rm ./dirsrvtests/tests/tickets/ticket47838_test.py I never want to see it again, it's a silly test. I think it adds no value and no help. @droideck may have a different view though :)

I agree with you. I think it makes sense to just get rid of this. But I think the decision is up to @vashirov to make

389-ds-bot commented 4 years ago

Comment from vashirov (@vashirov) at 2019-06-05 17:27:57

I'm OK with removing ticket47838_test.py.

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-06-11 08:52:42

rebased onto d01f1a1e176211df9df786c18f0e5fc08d6cfe54

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-06-11 09:15:42

3 new commits added

389-ds-bot commented 4 years ago

Comment from aadhikari at 2019-06-11 09:18:35

@Firstyear Changes regarding the replication manager's creation is fixed, please review the rest of the code.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-06-11 11:21:48

What does this option do? Could be good to comment this ....

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-06-11 11:24:16

Have a look at the function definition in replica.py: def __init__(self, instance, dn='cn=replication manager,cn=config'):. You don't need "defaultProperties[...]". Remember, lots of parts of lib389 are made to "do the right thing, in the simplest way"!