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

PR - Issue 49586 - Add py3 support to plugins test suite #2696

Closed
389-ds-bot opened this issue Sep 13, 2020 · 19 comments
Closed

PR - Issue 49586 - Add py3 support to plugins test suite #2696

389-ds-bot opened this issue Sep 13, 2020 · 19 comments
Labels
merged Migration flag - PR pr Migration flag - PR

Comments

@389-ds-bot
Copy link

389-ds-bot commented Sep 13, 2020

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

  • Created at 2018-04-13 10:33:28 by aadhikari
  • Merged at 2018-04-18 13:50:02

Description: Added py3 support by explicitly changing strings to bytes.
Added code for creating a new connection to make ldap communicate on localhost.

Resolves: #2645

@389-ds-bot 389-ds-bot added merged Migration flag - PR pr Migration flag - PR labels Sep 13, 2020
@389-ds-bot
Copy link
Author

Comment from aadhikari at 2018-04-13 10:38:22

rebased onto 219c2216ef041ab2ead5c0df9795fac95733667d

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2018-04-13 11:06:53

I think a more efficient way to fix the test suite is to fix the functions: _create_user, _create_group, etc. So they return the Bytes type values.
Fixing every variable through the tests seems not very efficient.

@389-ds-bot
Copy link
Author

389-ds-bot commented Sep 13, 2020

Comment from aadhikari at 2018-04-13 16:29:45

@droideck yes mate that make sense, but it if we change that in the function it will return values in bytes but it will fail in #2696#_4,74 as it will take 1st arg as string and won't take bytes.

@389-ds-bot
Copy link
Author

389-ds-bot commented Sep 13, 2020

Comment from spichugi (@droideck) at 2018-04-16 10:29:46

@droideck yes mate that make sense, but it if we change that in the function it will return values in bytes but it will fail in #2696#_4,74 as it will take 1st arg as string and won't take bytes.

The appearance of this thing is much less common than the one you've changed.
You can use 'ensure_str' there.

@389-ds-bot
Copy link
Author

Comment from aadhikari at 2018-04-16 15:51:48

rebased onto 10878617828a64715670dbb1758524d8cbcc077d

@389-ds-bot
Copy link
Author

Comment from aadhikari at 2018-04-16 16:13:09

rebased onto a174d537f85c896b2b9e6faa1759e678b58e102b

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2018-04-16 16:44:12

It doesn't make sense. You can leave None as it is because it has NoneType anyway.

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2018-04-16 16:45:33

Why did you remove 'assert' here?

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2018-04-16 16:47:29

some strange trailing whitespaces

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2018-04-16 16:49:07

As I mentioned before, it is better to change it in one place: in the return of the function. So in this case, _get_group_dn should return bytes.

@389-ds-bot
Copy link
Author

Comment from aadhikari at 2018-04-17 13:50:28

rebased onto fae125ee766f3fe133acdf7b8bf99902de02067b

@389-ds-bot
Copy link
Author

Comment from aadhikari at 2018-04-17 16:57:48

rebased onto 248642d366f36d63a8b9f30cab1f499c8d43b0ad

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2018-04-18 07:51:49

Ok, I see why we can remove the assert.
When we do 'assert Entry', it returns Falso when there is no data.
https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/_entry.py#_82

So it doesn't make sense to have the function in the test suite.

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2018-04-18 09:03:52

Extra print statement

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2018-04-18 09:12:29

Currently, we generate parameters for the instance here:
https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/utils.py#_850

We don't use constants as PORT_STANDALONE anymore. I think we should use all the info from DirSrv() object instance itself - like topology_st.standalone.port

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2018-04-18 09:45:36

Beside that, LGTM. The test suites pass.

@389-ds-bot
Copy link
Author

Comment from aadhikari at 2018-04-18 11:54:43

rebased onto e306a2d

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2018-04-18 13:50:03

Pull-Request has been merged by droideck

@389-ds-bot
Copy link
Author

Patch
49637.patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged Migration flag - PR pr Migration flag - PR
Projects
None yet
Development

No branches or pull requests

1 participant