Bug 1428047 - Require a Jenkins job to validate Change-ID on commits to branches in glusterfs repository
Summary: Require a Jenkins job to validate Change-ID on commits to branches in gluster...
Keywords:
Status: CLOSED WORKSFORME
Alias: None
Product: GlusterFS
Classification: Community
Component: project-infrastructure
Version: mainline
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: bugs@gluster.org
QA Contact:
URL:
Whiteboard:
Depends On:
Blocks: 1440805
TreeView+ depends on / blocked
 
Reported: 2017-03-01 18:59 UTC by Shyamsundar
Modified: 2019-05-27 01:49 UTC (History)
5 users (show)

Fixed In Version: glusterfs-3.12.0
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
: 1440805 1440810 (view as bug list)
Environment:
Last Closed: 2019-05-27 01:49:51 UTC
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:


Attachments (Terms of Use)

Description Shyamsundar 2017-03-01 18:59:17 UTC
If this[1] proposal goes through, then we would require a Jenkins job that does what is detailed in the mail. I.e,

- On non-master branches, validate if provided gerrit "Change-ID" is part of master or not,
  - If yes, then post a message on the commit stating the same
  - If not, then post a warning on the commit

- This job need not vote, as there maybe cases where this needs to be overridden, say for changes specific to that branch (like release notes for example)

Comment 1 Shyamsundar 2017-03-01 19:00:48 UTC
Marking this as NEEDINFO against myself, so that I can unblock it once feedback from the community is recieved over the devel list.

Also [1] in description is missing and is now posted here,

[1] http://lists.gluster.org/pipermail/gluster-devel/2017-March/052216.html

Comment 2 Worker Ant 2017-04-05 20:53:12 UTC
REVIEW: https://review.gluster.org/17004 (scripts: Update rfc.sh to check existance of Change-Id in backports) posted (#1) for review on master by Shyamsundar Ranganathan (srangana@redhat.com)

Comment 3 Shyamsundar 2017-04-05 20:58:39 UTC
The initial change to enforce/remind contributors to this requirement is to update rfc.sh. The commit in comment #2 is towards the same.

Once that is approved, I will possibly move this bug to the infra queue, for a Jenkins job that can post a comment to non-master commits when the Change-Id is not found in master.

The latter is to serve as a checkpoint for reviewers so that this aspect is not missed.

Comment 4 Worker Ant 2017-04-06 19:17:12 UTC
REVIEW: https://review.gluster.org/17004 (scripts: Update rfc.sh to check existance of Change-Id in backports) posted (#2) for review on master by Shyamsundar Ranganathan (srangana@redhat.com)

Comment 5 Worker Ant 2017-04-07 13:50:44 UTC
REVIEW: https://review.gluster.org/17004 (scripts: Update rfc.sh to check existance of Change-Id in backports) posted (#3) for review on master by Shyamsundar Ranganathan (srangana@redhat.com)

Comment 6 Worker Ant 2017-04-10 09:10:02 UTC
COMMIT: https://review.gluster.org/17004 committed in master by Niels de Vos (ndevos@redhat.com) 
------
commit 788def7912c68616849748678574c60a52021e3c
Author: Shyam <srangana@redhat.com>
Date:   Wed Apr 5 14:22:57 2017 -0400

    scripts: Update rfc.sh to check existance of Change-Id in backports
    
    Addition to this script is a no-op on master.
    
    This would need to be backported to active release branches to be
    effective.
    
    This check is not smart proof, in that someone could proceed knowing
    that the Change-Id differs from master, but this is not expected to
    catch that, instead it is to serve more as a reminder that we need
    the same Change-Id across branches.
    
    Contributors not using rfc.sh would not see this, but they are few
    and possibly far in between. Also contributors using gerrit to
    cherry-pick changes will not see this. For both cases a server side
    solution to catch any changes are needed.
    
    There is a possiblilty that we will follow this up with a check
    on the gerrit end and add a comment to the reviews, to aid reviewers
    to quickly check the sanity of the Change-Id when it differs.
    
    Change-Id: I11e371489a4a3cf2ff96d9892256986cd535998b
    BUG: 1428047
    Signed-off-by: Shyam <srangana@redhat.com>
    Reviewed-on: https://review.gluster.org/17004
    Smoke: Gluster Build System <jenkins@build.gluster.org>
    Reviewed-by: Niels de Vos <ndevos@redhat.com>
    NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org>
    CentOS-regression: Gluster Build System <jenkins@build.gluster.org>
    Reviewed-by: Kaleb KEITHLEY <kkeithle@redhat.com>
    Reviewed-by: Amar Tumballi <amarts@redhat.com>

Comment 7 Shyamsundar 2017-04-10 13:41:43 UTC
Moving this back to assigned, as we still need the Jenkins job to throw a warning on non-master review requests.

Comment 8 Shyamsundar 2017-04-10 14:48:12 UTC
The blocks is incorrect, as this bug continues for the Jenkins (or other) server side checks and will get closed only when that is done, whereas the 3.10 and 3.8 bugs filed for this are just for the rfc.sh change. As a result the blocking relationship does not arise IMO

Comment 9 Worker Ant 2017-05-30 07:44:57 UTC
REVIEW: https://review.gluster.org/17418 (scripts: prevent a script warning if rfc.sh does not find a backport) posted (#1) for review on master by Niels de Vos (ndevos@redhat.com)

Comment 10 Worker Ant 2017-05-30 13:02:26 UTC
COMMIT: https://review.gluster.org/17418 committed in master by Shyamsundar Ranganathan (srangana@redhat.com) 
------
commit f618c7e606caaf8371aea4a02b352c987b3b1a40
Author: Niels de Vos <ndevos@redhat.com>
Date:   Tue May 30 13:10:50 2017 +0530

    scripts: prevent a script warning if rfc.sh does not find a backport
    
    When running ./rfc.sh on a branch to post release notes (not a backport
    from master), then the following script warning is displayed:
    
       ./rfc.sh: line 97: [: =: unary operator expected
    
    In case the used variables are not set, they are not handled as empty
    strings. Placing the variables inside "${qoutes}" prevents this warning.
    
    BUG: 1428047
    Change-Id: Ie171d6f66b47401d6ea4e78aa3ed2bd0c6fce9ce
    Signed-off-by: Niels de Vos <ndevos@redhat.com>
    Reviewed-on: https://review.gluster.org/17418
    Smoke: Gluster Build System <jenkins@build.gluster.org>
    NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org>
    Reviewed-by: Kaleb KEITHLEY <kkeithle@redhat.com>
    CentOS-regression: Gluster Build System <jenkins@build.gluster.org>
    Reviewed-by: Shyamsundar Ranganathan <srangana@redhat.com>

Comment 11 Shyamsundar 2017-05-30 18:46:32 UTC
This bug is getting closed because a release has been made available that should address the reported issue. In case the problem is still not fixed with glusterfs-3.11.0, please open a new bug report.

glusterfs-3.11.0 has been announced on the Gluster mailinglists [1], packages for several distributions should become available in the near future. Keep an eye on the Gluster Users mailinglist [2] and the update infrastructure for your distribution.

[1] http://lists.gluster.org/pipermail/announce/2017-May/000073.html
[2] https://www.gluster.org/pipermail/gluster-users/

Comment 12 Shyamsundar 2017-05-30 19:09:03 UTC
Re-opening this, a Jenkins job to detect Change-ID anomolies would ease any discrepencies for reviewers and release-owners when merging changes, to understand and possibly educate on the need.

Comment 13 Shyamsundar 2017-09-05 17:25:47 UTC
This bug is getting closed because a release has been made available that should address the reported issue. In case the problem is still not fixed with glusterfs-3.12.0, please open a new bug report.

glusterfs-3.12.0 has been announced on the Gluster mailinglists [1], packages for several distributions should become available in the near future. Keep an eye on the Gluster Users mailinglist [2] and the update infrastructure for your distribution.

[1] http://lists.gluster.org/pipermail/announce/2017-September/000082.html
[2] https://www.gluster.org/pipermail/gluster-users/

Comment 14 Shyamsundar 2017-09-05 17:35:33 UTC
Keeps getting closed each time we make a release :)

Re-opening this, a Jenkins job to detect Change-ID anomolies would ease any discrepencies for reviewers and release-owners when merging changes, to understand and possibly educate on the need.

Comment 15 Shyamsundar 2017-09-05 17:35:54 UTC
reopening as per comment #14

Comment 16 Nigel Babu 2017-11-24 09:34:08 UTC
Can I have an example change and how our test should behave? I think we're ready to spend some time on this to get this moving.

Comment 17 Shyamsundar 2017-11-27 14:03:40 UTC
Here is an easy example that is in my mind:

- The rfc.sh already does this job and let's the committer know that the change ID is not present in master (https://github.com/gluster/glusterfs/blob/master/rfc.sh#L70)

- The same check needs to be performed, and a message posted to the patch, that the Change-ID is either inconsistent or missing

- The job need not vote, as at times having different change IDs or not finding a change ID on master is a valid case (release notes, release specific patches are examples where the change ID in the branch will not be found in master)

The intent of the job is simply to remind reviewers that the change ID is different and we may want to cross check that to ensure that it is genuine and not an error by the committer.

Does the above help? Or, do you need more here?

Comment 19 Amar Tumballi 2019-05-10 12:54:11 UTC
I guess for now, the work we have done through ./rfc.sh is good. Prefer to close as WORKSFORME.


Note You need to log in before you can comment on or make changes to this bug.