[Starlingx-discuss] Notes: Weekly StarlingX non-OpenStack Distro meeting, 12/12

[Liu, Changcheng]

Hi Dean,
Please check my below interleaved reply.
Thanks for your suggestion.

B.R.
Changcheng

From: Dean Troyer [mailto:dtroyer at gmail.com]
Sent: Friday, December 14, 2018 12:36 AM
To: Liu, Changcheng <changcheng.liu at intel.com>
Cc: Xie, Cindy <cindy.xie at intel.com>; starlingx-discuss at lists.starlingx.io
Subject: Re: [Starlingx-discuss] Notes: Weekly StarlingX non-OpenStack Distro meeting, 12/12

On Wed, Dec 12, 2018 at 8:06 PM Liu, Changcheng <changcheng.liu at intel.com<mailto:changcheng.liu at intel.com>> wrote:
    2. [Dean] the references to the original commit are not available externally
        [Changcheng] Yes. The reference should be removed from commit message at last.
        Originally, I want both Intel & WindRiver engineers could find where the patches are ported from which place at the initial porting stage.

[dt] The reference should NOT be removed.  If a patch is being rebased the reference to the original patch must be preserved.
[Changcheng]: I didn’t get your point. Commit message should be kept there. For the reference(how to find the original patch), I’ll keep them in the initial porting stage. Once we verified these ported patches works in the end, I’ll remove the reference since they can’t be accessed by external Intel/WindRiver engineers. Do you mean that we should keep “url link” in the commit message directly? Intel internal github can’t be accessed by external engineers.

3. [Dean] It also seems like it would be easier to review and merge these in smaller batches.
        [Changcheng] Yes. I’m syncing with WindRiver engineers to check whether we could merge some patches firstly to avoid times of rebase and review.

[dt] I am not talking about merging commits, I am talking about splitting the existing commits into multiple Github PRs.  Please do not merge commits that are not directly related to each other..
[Changcheng] Some patches are depended on previous patches. We could try to extract patches into new PR later. Currently, we need make sure ceph works locally. The PR could be accessed by WindRiver engineers who’re working with us to debug the problems.

    4. [Dean] There is also no reference in either the commit messages or the PR description to a Storyboard story or task or any further documentation to why this work is being done.
        [Changcheng] I’ll add related information in PR message if we agree with merge part of patches firstly.

[dt] Please put it into the commit messages.  The PR text is not part of the git repo and is lost github is unavailable.
[Changcheng] Yes. Right commit message should be kept there. If you find some commit message is lost, please tell give comment in the patch. Currently, I have kept all the commit message in the right place. For PR message, we could refine them to meet with your requirement.

    5. [Dean] only the git commit messages are guaranteed to stay with the code changes.
        [Changcheng] We’ll give document about stx-ceph upgrade once it’s been upgraded successfully.

[dt]That does not address the need to put good information into the individual commit messages.
[Changcheng] As I’ve said previously “If you find some commit message is lost, please tell give comment in the patch. Currently, I have kept all the commit message in the right place.”

6. [Dean] I had asked for the relevant information to be included in the individual commit messages and I still do not see that being done.  We are losing valuable information and traceability for why we are making these changes to upstream.
        [Changcheng] Personally, I think I’ve kept most part of original commit message in the new ported patches. Some huge patch is divided into small patches(If you look the original patch, it’s merged by several patches. It’s hard to be maintained). For PR info, we could give more detail info according to your requirement.

[dt]Thank you for splitting up previously squashed patches.  Please do not confuse PR information (the text that is part of a Github PR) with a commit message (the text that is part of a git commit).  Github PRs are not the place of record for us.  Information that does not fit into a git commit message should be in Storyboard or Launchpad, the two places we keep track of those things.  But more importantly, things that the next team that looks at this code will want to have without access to Github or Storyboard or Launchpad needs to be in the commit message.  This is exactly the problem we have with the existing patches agains upstream code where we have commitmessages sometimes with only a link to a ticketing system that we do not have access to.  I do not want that to continue.
[Changcheng] We could avoid much effort if we didn’t squash several patches into one big patch in stx-ceph/v10.2.2. Thanks for your reminding about the distinguish between PR info and patch commit message. For PR info, we’ll refine them with your requirement. For patch commit message, I haven’t found any serious problem. If you find something wrong, give comment in the patch directly.

dt

--

Dean Troyer
dtroyer at gmail.com<mailto:dtroyer at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.starlingx.io/pipermail/starlingx-discuss/attachments/20181214/4eaa007f/attachment.html>