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

ceph-disk: Compatibility fixes for Python 3 #9936

Merged
merged 8 commits into from Sep 8, 2016

Conversation

onyb
Copy link
Contributor

@onyb onyb commented Jun 26, 2016

No description provided.

@@ -4092,7 +4091,7 @@ def list_devices():

uuid_map = {}
space_map = {}
for base, parts in sorted(six.iteritems(partmap)):
for base, parts in sorted(partmap.items()):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you squash this commit into the previous one introducing six?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tchaikov Done.

@@ -18,6 +18,8 @@
# GNU Library Public License for more details.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: the commit message lists some commit subjects that you squashed - you can delete those

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdurgin: Done.

@jdurgin
Copy link
Member

jdurgin commented Jun 28, 2016

these fixes look good to me - as discussed on irc, I think we should use py34 as the python 3 env, and add python34 to install-deps.sh so 'make check' passes on centos 7 and ubuntu trusty

for line in proc_mounts:
fields = line.split()
if len(fields) < 3:
continue
mounts_dev = fields[0]
path = fields[1]
if mounts_dev.startswith('/') and os.path.exists(mounts_dev):
if mounts_dev.startswith(b'/') and os.path.exists(mounts_dev):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could use os.path.isabs(mounts_dev) instead of mounts_dev.startswith(b'/')

@tchaikov
Copy link
Contributor

tchaikov commented Jul 8, 2016

needs rebase.

@onyb onyb force-pushed the wip-ceph-disk-py3 branch 4 times, most recently from 07668c3 to 3e39ce2 Compare July 15, 2016 02:55
@@ -1987,6 +1987,7 @@ def prepare_file(self):
'not the same device as the osd data' %
self.name)
self.space_symlink = space_file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like space_symlink should be set to the path, not the file object

@jdurgin
Copy link
Member

jdurgin commented Aug 16, 2016

lgtm

@@ -213,7 +212,6 @@ def is_dmcrypt(ptype, name):
'upstart',
'sysvinit',
'systemd',
'openrc',
Copy link
Contributor

@tchaikov tchaikov Aug 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@onyb is this change on purpose? if yes, could you put the rationales in the commit message? and i guess it's not part of the pep8ifying.

@jdurgin
Copy link
Member

jdurgin commented Aug 16, 2016

tests passed, but this needs a rebase now

ceph-disk: Misc cleanups

Signed-off-by: Anirudha Bose <ani07nov@gmail.com>
Signed-off-by: Anirudha Bose <ani07nov@gmail.com>
Python fcntl.lockf() accepts a file descriptor, not a file object

Signed-off-by: Anirudha Bose <ani07nov@gmail.com>
Signed-off-by: Anirudha Bose <ani07nov@gmail.com>
acquire and release methods of FileLock are dropped

Signed-off-by: Anirudha Bose <ani07nov@gmail.com>
Signed-off-by: Anirudha Bose <ani07nov@gmail.com>
Signed-off-by: Anirudha Bose <ani07nov@gmail.com>
Signed-off-by: Anirudha Bose <ani07nov@gmail.com>
@onyb
Copy link
Contributor Author

onyb commented Aug 17, 2016

@jdurgin Rebased and addressed all comments by @tchaikov . Thanks!

@tchaikov
Copy link
Contributor

lgtm.

@tchaikov
Copy link
Contributor

tchaikov commented Sep 8, 2016

@jdurgin is this PR still under testing? oh,

tests passed, but this needs a rebase now

good to merge then, i guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants