OpenStack Identity (Keystone)

'Authors' check in run_tests.sh makes life harder for new contributers

Reported by Cole Robinson on 2012-01-24
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Glance
Wishlist
Bhuvan Arumugam
Keystone
Undecided
Unassigned
OpenStack Compute (nova)
Wishlist
Monty Taylor
OpenStack Core Infrastructure
High
Bhuvan Arumugam
OpenStack Dashboard (Horizon)
Wishlist
Unassigned
OpenStack Object Storage (swift)
Wishlist
Unassigned

Bug Description

SInce gerrit commits are now gated on nova tests passing, the unit test that checks for an up to date Authors file should really be dropped.

Consider the scenario I am currently in

- I've never committed to nova before so I am not in Authors
- I have 2 independent changes I want to submit to nova

My options are:

- Submit both changes independently, both updating Authors to appease the tests. If both are accepted, one gets in first and the other has a conflict, requiring me to resend.

- Submit one change as the parent of another. This is confusing to reviewers since the changes are orthogonal, and one change is blocked on the other being accepted/committed.

- Submit one change, sit on the other until the first is resolved, then submit the second fix.

That's all stop energy for not a lot of benefit that I can see. The preferred way would be:

- Submit both changes independently and not worry about updating Authors.

If yall wanted to get fancy there is probably some git hook that can be used to auto update Authors. Simpler would be to just have the release manager regenerate Authors as part of cutting a release.

Thierry Carrez (ttx) wrote :

I think Authors needs to be in sync with tree contents at any time, and not just for cosmetic reasons. Submitting one change as the parent of the other doesn't sound like too much of a hassle, to keep that benefit.

A git hook could replace it, but without .mailmap it can quickly turn the Authors file into something ugly.. and I'm not sure our Gerrit system would support that.

Adding openstack-ci for more comments.

Changed in nova:
status: New → Incomplete
Cole Robinson (crobinso) wrote :

I assume the implication is that there are legal reasons for this, otherwise really what _is_ the benefit over something like 'git log' (and even in the legal case why isn't 'git log' sufficient)?

Dave Walker (davewalker) wrote :

It's a valid point, the git log (with suitable --format) can create an accurate list of all Authors. What benefit does a .mailmap add? I agree the .mailmap shrinking will be lost.. but really.. in years to come, will we *really* enjoy having a list of 1000's of entries anyway?

On 02/10/2012 03:18 PM, Dave Walker wrote:
> It's a valid point, the git log (with suitable --format) can create an
> accurate list of all Authors. What benefit does a .mailmap add? I
> agree the .mailmap shrinking will be lost.. but really.. in years to
> come, will we *really* enjoy having a list of 1000's of entries anyway?

Also, now that we've got Gerrit gating CLA signature, the git log is
going to be really accurate. I kind of feel like the Authors check is a
hold solution for a problem we don't have any more.

How about we remove the Authors file altogether and have setup.py sdist
generate it from git?

Monty

Jay Pipes (jaypipes) wrote :

Agreed with Monty here. If we can automate the creation of an Authors file (now that Gerrit is gating on CLA signature) I think that makes much more sense.

Thierry Carrez (ttx) wrote :

I suppose git log will create multiple entries for the same person using multiple email addresses in git commits... The .mailmap reduction allowed us to have a sane list. It's used for PTL election purposes, so it's good to have one entry = one person.

Or is there another way to automagically reduce the list to current email addresses ?

2012/2/20 Thierry Carrez <email address hidden>:
> I suppose git log will create multiple entries for the same person using
> multiple email addresses in git commits... The .mailmap reduction
> allowed us to have a sane list. It's used for PTL election purposes, so
> it's good to have one entry = one person.
>
> Or is there another way to automagically reduce the list to current
> email addresses ?

git log already respects .mailmap (that's where I got the idea when I
wrote the implementation for the check back when we used bzr), so it
should "Just Work[tm]".

Thierry Carrez (ttx) wrote :

So we could... keep .mailmap but autogenerate Authors ? Sounds like a good way to generate duplicate entries for extra addresses missing a .mailmap entry...

Bhuvan Arumugam (bhuvan) wrote :

Filed bug 976267 to track the change related to auto-generating AUTHORS file. When it's in place, we could remove the test test_authors.py, that check for an entry in AUTHORS file.

Bhuvan Arumugam (bhuvan) wrote :

Proposed a patch for review, to remove this test case. As per bug 976267, a patch is in queue to auto generate AUTHORS file.
  https://review.openstack.org/6701

For now, i've posted the patch for keystone.

I've posted patches for bug 976267 to auto generate AUTHORS file. Once those patches are merged, I'll propose patches to remove this test case, for other components.

Bhuvan Arumugam (bhuvan) wrote :

The code to auto generate AUTHORS file and remove the test case has been merged for glance component.
  https://review.openstack.org/#/c/6698/

Monty Taylor (mordred) on 2012-05-05
Changed in openstack-ci:
status: New → In Progress
assignee: nobody → Bhuvaneswaran A (bhuvan)
milestone: none → folsom
Monty Taylor (mordred) on 2012-05-06
Changed in openstack-ci:
importance: Undecided → High
Bhuvan Arumugam (bhuvan) wrote :

The test case is removed/merged from following projects:
  * keystone
  * glance
  * openstack-common
  * python-openstackclient

The patch for review has been posted for following projects:
  * nova https://review.openstack.org/#/c/6699/

I'm working on a patch to remove this test case for following projects:
  * quantum
  * horizon
  * swift

Changed in keystone:
status: New → Fix Committed
Changed in glance:
status: New → Fix Committed
Changed in nova:
status: Incomplete → In Progress
Changed in horizon:
importance: Undecided → Wishlist
status: New → Confirmed
Joseph Heck (heckj) on 2012-05-20
Changed in keystone:
milestone: none → folsom-1
Brian Waldon (bcwaldon) on 2012-05-22
Changed in glance:
importance: Undecided → Wishlist
Thierry Carrez (ttx) on 2012-05-23
Changed in keystone:
status: Fix Committed → Fix Released
Thierry Carrez (ttx) on 2012-05-23
Changed in glance:
milestone: none → folsom-1
status: Fix Committed → Fix Released
Brian Waldon (bcwaldon) on 2012-05-24
Changed in glance:
assignee: nobody → Bhuvaneswaran A (bhuvan)
Bhuvan Arumugam (bhuvan) on 2012-06-04
Changed in openstack-ci:
status: In Progress → Fix Committed
Thierry Carrez (ttx) on 2012-06-07
Changed in nova:
assignee: nobody → Bhuvaneswaran A (bhuvan)
importance: Undecided → Wishlist
Changed in swift:
importance: Undecided → Wishlist
status: New → Confirmed

Fix proposed to branch: master
Review: https://review.openstack.org/9369

Changed in nova:
assignee: Bhuvaneswaran A (bhuvan) → Monty Taylor (mordred)

Reviewed: https://review.openstack.org/9369
Committed: http://github.com/openstack/nova/commit/3456b667c38aa1d439482b9c95838aba6d3d6c5c
Submitter: Jenkins
Branch: master

commit 3456b667c38aa1d439482b9c95838aba6d3d6c5c
Author: Monty Taylor <email address hidden>
Date: Thu Jul 5 09:08:38 2012 -0500

    Finish AUTHORS transition.

    The code to generate the authors file from the git changelog has been in the
    tree and running for a few weeks now. Somehow the removal of the authors
    test and and the MANIFEST.in file were wrong though. This should clean that
    up.

    Fixes bug 920757.

    Change-Id: I66c388c1c81804f8dabc52b5ee25c7f394921e11

Changed in nova:
status: In Progress → Fix Committed
Monty Taylor (mordred) on 2012-07-06
Changed in nova:
milestone: none → folsom-3
Changed in horizon:
status: Confirmed → Fix Released
Monty Taylor (mordred) on 2012-07-28
Changed in openstack-ci:
status: Fix Committed → Fix Released
Thierry Carrez (ttx) on 2012-08-16
Changed in nova:
status: Fix Committed → Fix Released
Thierry Carrez (ttx) on 2012-09-27
Changed in glance:
milestone: folsom-1 → 2012.2
Thierry Carrez (ttx) on 2012-09-27
Changed in keystone:
milestone: folsom-1 → 2012.2
Thierry Carrez (ttx) on 2012-09-27
Changed in nova:
milestone: folsom-3 → 2012.2
Samuel Merritt (torgomatic) wrote :

There's no AUTHORS check in Swift's tests, so I'm guessing it got removed at some point and this bug didn't get updated.

Changed in swift:
status: Confirmed → Fix Released
John Dickinson (notmyname) wrote :

Actually, there was never an authors check in swift. Same overall result, though.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers