Skip to content

doc: split up SubmittingPatches.rst#30705

Merged
smithfarm merged 1 commit intoceph:masterfrom
smithfarm:wip-sp-backporting-2
Oct 19, 2019
Merged

doc: split up SubmittingPatches.rst#30705
smithfarm merged 1 commit intoceph:masterfrom
smithfarm:wip-sp-backporting-2

Conversation

@smithfarm
Copy link
Contributor

@smithfarm smithfarm commented Oct 3, 2019

(#22876 was too cluttered, so I replaced it with this fresh PR)

Split the existing SubmittingPatches.rst into three files:

  • SubmittingPatches.rst (general guidelines, for GitHub/master branch)
  • SubmittingPatches-kernel.rst (for kernel patches)
  • SubmittingPatches-backports.rst (for backports)

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


TODO:

Copy link
Member

@liewegas liewegas left a comment

Choose a reason for hiding this comment

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

I saw one typo; otherwise this is fantastic!

@smithfarm smithfarm force-pushed the wip-sp-backporting-2 branch from a531eb1 to db5c8b7 Compare October 3, 2019 14:58
@smithfarm
Copy link
Contributor Author

@liewegas Thanks for the review. The typo should be fixed now, and I also pushed a second commit which adds a "Test your changes" section to SubmittingPatches.rst if you care to take a look.

For example::

osd: make the ClassHandler::mutex private
Fixes: 9dbe7a003989f8bb45fe14aaa587e9d60a392727
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't we have ever practised this, at least not in the main repo. Not saying that this is a bad idea, just that it doesn't codify existing practice and should be discussed on the mailing list first for it to get any traction. Because everyone is doing their own thing: I noticed @batrick has been using Introduced-by, I usually put an abbreviated SHA1 along with the title somewhere in the commit message, etc.

Copy link
Member

Choose a reason for hiding this comment

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

We recently started making a point of mentioning the commit that introduced the problem to help identify follow-on fixes to backport, but I don't think we standardized on the Prefix: that precedes it. Fixes: seems appropriate though? In any case, this is a good time to go ahead codify this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@idryomov This format was suggested by Sage quite some months (or even a year or more) ago.

Copy link
Member

Choose a reason for hiding this comment

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

I'm down with this format.

Copy link
Member

Choose a reason for hiding this comment

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

+1

@smithfarm smithfarm force-pushed the wip-sp-backporting-2 branch 2 times, most recently from 2b2d938 to 3a0da5d Compare October 4, 2019 13:41
@smithfarm
Copy link
Contributor Author

@idryomov Thanks for the review. I think I have addressed all the comments.

Copy link
Member

@gregsfortytwo gregsfortytwo left a comment

Choose a reason for hiding this comment

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

Reviewed-by: Greg Farnum gfarnum@redhat.com

Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

LGTM apart from some nits.

Copy link
Contributor

@LenzGr LenzGr left a comment

Choose a reason for hiding this comment

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

Apart from some minor things that I commented on, this is a great improvement. Thanks a lot!

Split the existing SubmittingPatches.rst into three files:

* SubmittingPatches.rst (general guidelines, for GitHub/master branch)
* SubmittingPatches-kernel.rst (for kernel patches)
* SubmittingPatches-backports.rst (for backports)

Fixes: http://tracker.ceph.com/issues/20953
Signed-off-by: Nathan Cutler <ncutler@suse.com>
@smithfarm smithfarm force-pushed the wip-sp-backporting-2 branch from 55ed784 to ec6af09 Compare October 17, 2019 17:32
@smithfarm smithfarm force-pushed the wip-sp-backporting-2 branch 10 times, most recently from dadbe1f to ec6af09 Compare October 19, 2019 08:11
@smithfarm
Copy link
Contributor Author

Thanks everyone for reviewing. There is still a lot that can be improved. I'm in the process of making further revisions - especially to SubmittingPatches-backports.rst, but I moved them to a new branch and will open a follow-up PR. I reverted this branch to ec6af09 - the most recently reviewed SHA1.

@smithfarm smithfarm merged commit 63f984a into ceph:master Oct 19, 2019
@smithfarm smithfarm deleted the wip-sp-backporting-2 branch October 19, 2019 08:20
smithfarm added a commit to smithfarm/ceph that referenced this pull request Oct 19, 2019
After putting all the reviewers through ceph#30705
I realized there is still much to be improved in this document.

Signed-off-by: Nathan Cutler <ncutler@suse.com>
smithfarm added a commit to smithfarm/ceph that referenced this pull request Oct 19, 2019
After putting all the reviewers through ceph#30705
I realized there is still much to be improved in this document.

Signed-off-by: Nathan Cutler <ncutler@suse.com>
smithfarm added a commit to smithfarm/ceph that referenced this pull request Oct 24, 2019
After putting all the reviewers through ceph#30705
I realized there is still much to be improved in this document.

Signed-off-by: Nathan Cutler <ncutler@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.