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

Thu Dec 13 02:04:10 UTC 2018

Hi Dean,
    1. [Dean] Notice how the second paragraph of the original message is missing from the new commit.
        [Changcheng] The second paragraph isn't missed in the new patch.
        You could find: " RevertMe: " & " This commit should be reverted when we decide to enable the ceph user."

    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.

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.

    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.

    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.

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.

B.R.
Changcheng

[X]
-----Original Message-----
From: Dean Troyer [mailto:dtroyer at gmail.com]
Sent: Thursday, December 13, 2018 12:20 AM
To: Xie, Cindy <cindy.xie at intel.com>
Cc: 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:47 AM Xie, Cindy <cindy.xie at intel.com<mailto:cindy.xie at intel.com>> wrote:
>     3. Ceph upgrade status (Vivian/Dehao/Changcheng) Dean merged all
> the build process patche (on staging stx-ceph and Changcheng finish
> rebasing all patches according to latest stx-ceph (stx/v13.2.2).
> totally 17 PRs New PR: https://github.com/starlingx-staging/stx-ceph/pull/18 pending for review.

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.  Let's look at an example:

In [0] we have the following commit message:

----------
Port: RevertMe: Use user root to run ceph services

Avoid debugging file permission issues when upgrading to Jewel.
This is done to provide the same setup as Hammer in StarlingX.
This commit should be reverted when we decide to enable the ceph user.

Port From:
    Ceph Rebase: Disable ceph user/group for Hammer equivalence.patch
        0001____src_ceph-disk_ceph_disk_main.py.patch
0002____src_init-ceph.in.patch
0003____wrs_ceph.conf.patch

Signed-off-by: Robert Church <robert.church at windriver.com<mailto:robert.church at windriver.com>>
Signed-off-by: Daniel Badea <daniel.badea at windriver.com<mailto:daniel.badea at windriver.com>>
Signed-off-by: Changcheng Liu <changcheng.liu at intel.com<mailto:changcheng.liu at intel.com>>
Signed-off-by: Dehao Shang <dehao.shang at intel.com<mailto:dehao.shang at intel.com>>
----------

This appears to correspond to the original R5 commit c87de31f that has the following commit message:

----------
    Ceph Rebase: Disable ceph user/group for Hammer equivalence

    Use default (root) user to run ceph services instead of dedicated (ceph)
    user and group to avoid debugging file permission issues while upgrading
    to Jewel.

    This is done to provide the same setup as Hammer in TiS. This commit
    should be reverted when we decide to enable the ceph user.
----------

Notice how the second paragraph of the original message is missing from the new commit.

Also, the references to the original commit are not available externally, I have no idea what "0001____src_ceph-disk_ceph_disk_main.py.patch" refers to.  So even for someone with access to the original commit I have to do text string searches to attempt to locate it in the R5 repo.

It also seems like it would be easier to review and merge these in smaller batches.  One big PR with 35 commits takes time to review, and when a single change needs to be made we have to re-review looking for the changes.

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.

Think of what you have available while doing this rebase/upgrade and imagine what the next person doing the next rebase/upgrade will want to see and make sure all of that is present in the commit messages.
The GitHub PR may or may not be available at that time, only the git commit messages are guaranteed to stay with the code changes.

dt

[0] https://github.com/starlingx-staging/stx-ceph/pull/18/commits/552736f77f39897922e562a8477d19ab4e47a39f

--

Dean Troyer
dtroyer at gmail.com<mailto:dtroyer at gmail.com>

_______________________________________________
Starlingx-discuss mailing list
Starlingx-discuss at lists.starlingx.io<mailto:Starlingx-discuss at lists.starlingx.io>
http://lists.starlingx.io/cgi-bin/mailman/listinfo/starlingx-discuss

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.starlingx.io/pipermail/starlingx-discuss/attachments/20181213/17b3ebad/attachment-0001.html>