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: change ownership of initfile to ceph:ceph #9688

Merged
1 commit merged into from Aug 16, 2016
Merged

ceph-disk: change ownership of initfile to ceph:ceph #9688

1 commit merged into from Aug 16, 2016

Conversation

shylesh
Copy link
Contributor

@shylesh shylesh commented Jun 14, 2016

Signed-off-by: shylesh kumar shylesh.mohan@gmail.com

@tchaikov tchaikov changed the title Tracker 16280:-change ownership of initfile to ceph:ceph ceph-disk: Tracker 16280:-change ownership of initfile to ceph:ceph Jun 14, 2016
@tchaikov
Copy link
Contributor

@shylesh could you add a

Fixes: http://tracker.ceph.com/issues/16280

line right before your Signed-off-by line in your commit message?

and could you prefix the title of your commit message with the subcomponent your are changing ? see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#preparing-and-sending-patches. in this case, it would be "ceph-disk: ".

@shylesh
Copy link
Contributor Author

shylesh commented Jun 14, 2016

@tchaikov

Done, Thanks for the input kefu

LOG.debug('Marking with init system %s', init)
init_path = os.path.join(path, init)
with file(init_path, 'w'):
path_set_context(init_path)
Copy link

Choose a reason for hiding this comment

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

it should be indented to the right so that it is included in the if instead of being outside. Otherwise looks good and ready for tests ;-)

@shylesh
Copy link
Contributor Author

shylesh commented Jun 14, 2016

@tchaikov ,

Sorry , some problem with vim tabs. Done

@tchaikov
Copy link
Contributor

tchaikov commented Jul 7, 2016

could you remove "-Tracker 16280-" from the title of your commit message? and it should be good.

@shylesh
Copy link
Contributor Author

shylesh commented Jul 7, 2016

Done

@tchaikov tchaikov changed the title ceph-disk: Tracker 16280:-change ownership of initfile to ceph:ceph ceph-disk: change ownership of initfile to ceph:ceph Jul 7, 2016
@tchaikov
Copy link
Contributor

tchaikov commented Jul 7, 2016

lgtm.

@tchaikov
Copy link
Contributor

tchaikov commented Jul 7, 2016

@shylesh could you add this back and keep the title of the commit message unchanged?

Fixes: http://tracker.ceph.com/issues/16280

@shylesh
Copy link
Contributor Author

shylesh commented Jul 7, 2016

@tchaikov ,
Done

@ghost
Copy link

ghost commented Jul 7, 2016

the rocksdb submodule was updated by accident

@tchaikov tchaikov removed the needs-qa label Jul 7, 2016
@shylesh
Copy link
Contributor Author

shylesh commented Jul 7, 2016

@dachary ,

Done

@tchaikov
Copy link
Contributor

tested in http://pulpito.ceph.com/kchai-2016-07-10_07:48:29-rados-wip-kefu-testing---basic-mira/

the non-environmental failures are addressed by #10234 .

@tchaikov
Copy link
Contributor

@dachary is this PR good to merge?

@tchaikov
Copy link
Contributor

lgtm.

@ghost
Copy link

ghost commented Jul 11, 2016

it looks good, just needs to fix the tox errors

@tchaikov
Copy link
Contributor

tchaikov commented Aug 1, 2016

retest this please

@shylesh
Copy link
Contributor Author

shylesh commented Aug 2, 2016

@tchaikov ,

Let me know what tests needs to be run.

Thanks,
Shylesh

@@ -3245,8 +3245,9 @@ def activate(
init = init_get()

LOG.debug('Marking with init system %s', init)
with file(os.path.join(path, init), 'w'):
pass
init_path = os.path.join(path,init)
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a space after ","

@tchaikov
Copy link
Contributor

tchaikov commented Aug 2, 2016

sorry, "retest this please" is the command to trigger the build job on jenkins.

and btw, you might want to run "run-tox-ceph-disk" locally to fix the failure, see https://jenkins.ceph.com/job/ceph-pull-requests/9898/consoleFull#231334942c19247c4-fcb7-4c61-9a5d-7e2b9731c678

     13 - run-tox-ceph-disk (Failed)

@shylesh
Copy link
Contributor Author

shylesh commented Aug 16, 2016

@tchaikov ,
I did the change and repushed it. Anything else needs to be done ?

@tchaikov
Copy link
Contributor

lgtm @dachary what do you think?

@ghost ghost merged commit 10e9a27 into ceph:master Aug 16, 2016
@ghost
Copy link

ghost commented Aug 16, 2016

@tchaikov sorry, my thumb slipped after pasting my reviewed-by and merged. I should have added your reviewed-by as well :-(
@shylesh good patch :-)

@tchaikov
Copy link
Contributor

@dachary no worries =)

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants