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-volume: Try to cast OSD metadata to int while scanning directory #19477

Merged
merged 1 commit into from Dec 14, 2017

Conversation

Projects
None yet
2 participants
@wido
Copy link
Member

commented Dec 13, 2017

By doing so values like 'whoami' and 'bluefs' will be stored as a
integer in the resulting JSON rather then a String.

Signed-off-by: Wido den Hollander wido@42on.com

ceph-volume: Try to cast OSD metadata to int while scanning directory
By doing so values like 'whoami' and 'bluefs' will be stored as a
integer in the resulting JSON rather then a String.

Signed-off-by: Wido den Hollander <wido@42on.com>

@wido wido added the ceph-volume label Dec 13, 2017

@wido wido requested a review from alfredodeza Dec 13, 2017

@alfredodeza

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2017

I understand those are ints, but they aren't used as ints anywhere though. All through the activation process, we are dealing with strings with no necessity of casting. Do you foresee a problem (or maybe you are having an issue already) ?

@wido

This comment has been minimized.

Copy link
Member Author

commented Dec 14, 2017

Well, it's a puristic thing from me as well. You never know if there is somebody who wants to parse that JSON files in it's own tooling and would then have to cast everything again.

By making sure we write a properly typed JSON file we catch those cases already.

It's just me, when something is a Int, store it as a int.

@alfredodeza

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2017

@wido can you push this branch to ceph-ci.git so that we can run some tests? The changes look OK to me.

@wido

This comment has been minimized.

Copy link
Member Author

commented Dec 14, 2017

Done @alfredodeza

There is mgr in the branch name, but that's a mistake from my side. I was working on Mgr at the same time.

Here it is: https://github.com/ceph/ceph-ci/tree/mgr-int-json

@alfredodeza

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2017

jenkins test ceph-volume simple all

@alfredodeza alfredodeza merged commit d33f42b into ceph:master Dec 14, 2017

8 of 9 checks passed

make check (arm64) make check failed
Details
Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
ceph-volume simple testing centos7-bluestore-activate ceph-volume simple centos7-bluestore-activate OK
Details
ceph-volume simple testing centos7-filestore-activate ceph-volume simple centos7-filestore-activate OK
Details
ceph-volume simple testing xenial-bluestore-activate ceph-volume simple xenial-bluestore-activate OK
Details
ceph-volume simple testing xenial-filestore-activate ceph-volume simple xenial-filestore-activate OK
Details
make check make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.