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
PR - Added python3 support and code fixes. [WIP] #3346
Comments
Comment from firstyear (@Firstyear) at 2019-03-19 01:33:09 Don't duplicate this, use strcmdline = " ".join(cmdline) instead. |
Comment from firstyear (@Firstyear) at 2019-03-19 01:33:30 Sleep? Is there a better way? |
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 .... |
Comment from firstyear (@Firstyear) at 2019-03-19 01:34:19 And here. |
Comment from firstyear (@Firstyear) at 2019-03-19 01:34:26 And here. |
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. |
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 .... |
Comment from aadhikari at 2019-03-19 06:52:23
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. |
Comment from aadhikari at 2019-03-19 07:49:48
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 :) |
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). |
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. |
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? |
Comment from mhonek (@kenoh) at 2019-03-19 10:57:51 AFAICT this file's change effectively removes a test for a feature from cab38f9 introduced just a few weeks ago. What is the story here? |
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) |
Comment from aadhikari at 2019-03-19 14:02:46 rebased onto 4737790b289136ab3d3e65dfa04626f09b28815d |
Comment from aadhikari at 2019-03-19 14:41:23 1 new commit added
|
Comment from aadhikari at 2019-03-19 14:57:51 5 new commits added
|
Comment from aadhikari at 2019-03-19 15:05:54 5 new commits added
|
Comment from aadhikari at 2019-03-19 15:50:06 5 new commits added
|
Comment from aadhikari at 2019-03-19 16:10:50 rebased onto 024aa5b302ffa054f65dc54520040f27517514fb |
Comment from aadhikari at 2019-03-19 16:15:28
Was having some merge conflict issue, resolved and fixed. also, meaningful commit message is been added. |
Comment from aadhikari at 2019-03-19 17:27:39
@kenoh But I see them passing in the latest builds will remove it from the commit. |
Comment from aadhikari at 2019-03-19 18:07:34 rebased onto 8b7328b3d9947db82e8cbdba63cb1dd839c4a832 |
Comment from aadhikari at 2019-03-19 18:16:58
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. |
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? |
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 |
Comment from aadhikari at 2019-03-20 08:32:50
@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: |
Comment from firstyear (@Firstyear) at 2019-06-11 11:28:17 setup-ds.pl is deprecated, and may cause this test to break. What does the update do that you require? Consider running your test with --disable-perl on ./configure, and youll see where perl breaks things. If anything we shouldn't use def runUpgrade as a result .... |
Comment from aadhikari at 2019-06-12 06:38:44
the strcmdline is just for printing what we are executing if you see the whole code will give a better idea, BTW this change is done by your suggestion only.
|
Comment from aadhikari at 2019-06-12 06:39:59
This is already being mentioned in the previous comments(At the start of the test), if that what you are asking? |
Comment from aadhikari at 2019-06-12 06:54:50
@Firstyear I think, setup-ds.pl is deprecated but it is still consumed by RHEL 7 and many other OS and these will still be present for a very long time. Rather than using only the latest way, I think ds_older check can be used here but we still need to fix runUpgrade. Tell me a better way of doing this because we cannot ignore older versions. BTW is there anything similar to runUpgrade in lib389 for the the dscreate? because that can be used here with ds_check. |
Comment from aadhikari at 2019-06-12 06:59:56
Oops, fixing these changes right away! |
Comment from aadhikari at 2019-06-12 08:04:43
I think cn is needed, getting this: ldap.UNWILLING_TO_PERFORM: Attribute cn must not be None |
Comment from aadhikari at 2019-06-12 08:23:18 rebased onto 96a37764e0267187f1ef10ae1e35b8fa61bc1ef8 |
Comment from firstyear (@Firstyear) at 2019-06-12 09:33:39
Should we have a version check in runUpgrade then to check if it's 1.3.x or lower?
1.4.x, upgrades should be done "inside the server" at startup, else we fail to work with docker. Today, I don't think we have a fully agreed mechanism for this, but there is inplace an internal upgrade system already in ns-slapd so simply restarting is sufficient. But we may need to discuss and clarify this as a team to be completely sure. As for the cn being needed, yep, ignore my comment then :) I always thought that interface was simpler to use, so may I have forgotten something. And I think you answered my other questions. Thanks! |
Comment from aadhikari at 2019-06-17 09:10:15
Correct a check in the upgrade like if it is 1.3.x then do the things else just pass? Do you think that is fine? But in that case also we need to fix the byte issue inside runUpgrade correct?
|
Comment from firstyear (@Firstyear) at 2019-06-18 09:01:17
Yes I think that would be fine. |
Comment from aadhikari at 2019-06-19 11:40:36 rebased onto 570a2d12281884b023b4a43f404fff9d436174ed |
Comment from aadhikari at 2019-06-19 11:46:18 @Firstyear ds_is_older('1.4.0') is added so that all older version of DS can use the script, not sure in which version exactly it will be removed, as I see 1.4 version have a legacy tool package which can be used as if now, but it will be removed in future. So should we do this in this way or make it work specifically in 1.3.x or lower? |
Comment from firstyear (@Firstyear) at 2019-06-19 14:15:06 No - you may not call any perl tool from lib389 for 1.4.x. There is a "--disable-perl" flag in the ./configure, and no pl tools will be generated or emited. You must assume all 1.4.x installs run with this setting, and the pl tools as "legacy" are just for installing/removing/admin tools. So it must only call any pl tools with 1.3.x or lower. |
Comment from aadhikari at 2019-06-19 15:24:11
@Firstyear I did ds_is_older('1.4.0') that means it cover our requirement correct? |
Comment from aadhikari at 2019-06-19 15:24:44 @Firstyear for 1.4.x it won't run the script. |
Comment from firstyear (@Firstyear) at 2019-06-19 15:47:19 Great! that's what I wanted to hear. |
Comment from aadhikari at 2019-06-20 11:01:52 @Firstyear So everything is good in the patch? BTW, new findings, I ran the tests on a machine having 1.4 just to be sure, so it didn't run the update script as expected but the test failed. Seems like the internal upgrade system in ns-slapd is not working, I have restarted the server as mentioned by you. Any idea? |
Comment from firstyear (@Firstyear) at 2019-06-20 11:38:48 @aadhikari The internal upgrade does work, if you are finding something missing, then it means we need to port/add the upgrade requirements to the server core. So you'll need to report what the upgrade is meant to do, and how it's meant to work into an issue so we can implement it in the main server code. |
Comment from aadhikari at 2019-06-24 09:50:11 @Firstyear Here is the trace log and the whole run of the test, it failed to find AES plugin: No such object
|
Comment from aadhikari at 2019-06-24 14:22:55 rebased onto 1afde987e7aa931140f47ceb7e8c4e063d70299e |
Comment from aadhikari at 2019-07-09 09:11:02 rebased onto 4dbdbfac1ddb5e2db96d0b7024e2190dd41ef4e2 |
Comment from aadhikari at 2019-07-09 09:21:30 A better reason message can be suggested. |
Comment from aadhikari at 2019-07-09 09:22:50 2 new commits added
|
Comment from aadhikari at 2019-08-13 12:32:00 rebased onto 55112be2da7eda254f080d5657d440c87f22f573 |
Comment from aadhikari at 2019-08-13 15:09:21 rebased onto ca5f54bf78bd6c5c89e73f44ece460d50835042c |
Comment from aadhikari at 2019-08-13 15:22:18 rebased onto 7a24286 |
Comment from vashirov (@vashirov) at 2019-08-13 15:28:47 Pull-Request has been merged by vashirov |
Patch |
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: ??
The text was updated successfully, but these errors were encountered: