Add a tox test to ensure every pull request has a d/changelog entry

Bug #1655671 reported by Barry Warsaw
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Ubuntu Image
Fix Released
Medium
Łukasz Zemczak

Bug Description

It's mistake-prone to not include a d/changelog entry for every pull request that has a Launchpad bug. We will need a way to flag PRs that don't have a Launchpad bug (e.g. some minor typo fix or such), but those should be the exceptions.

Tags: tech-debt
Barry Warsaw (barry)
Changed in ubuntu-image:
assignee: nobody → Barry Warsaw (barry)
milestone: none → 0.14
Barry Warsaw (barry)
tags: added: tech-debt
Barry Warsaw (barry)
Changed in ubuntu-image:
milestone: 0.14 → 0.15
Barry Warsaw (barry)
Changed in ubuntu-image:
milestone: 0.15 → none
Revision history for this message
Barry Warsaw (barry) wrote :

Targeting this to 1.0, but I won't let it block so it's possible I'll de-milestone it later.

Changed in ubuntu-image:
milestone: none → 1.0
status: New → Confirmed
importance: Undecided → Medium
Revision history for this message
Łukasz Zemczak (sil2100) wrote :

Could I take a look at this maybe?

Revision history for this message
Łukasz Zemczak (sil2100) wrote :

Do you have an overall idea on how to get this done? It should be fairly simple to write a test like this that would be only executed on the PR CI case, but I would probably need to know some details about how our github CI works. Where could I get more info on that? Where should I look?

Revision history for this message
Barry Warsaw (barry) wrote :

Our GH CI is basically autopkgtest, and its exactly what gets run on autopkgtest.ubuntu.com for package promotion (which is nice, because you know if it passes our CI, barring any weirdness, it will promote).

To run our ci, just create a source package (e.g. using `gbp buildpackage -S`) and then run `autopkgtest ubuntu-image_*.dsc`. For best results, use autopkgtest-buildvm-ubuntu-cloud to create a bootable image of whatever distroversion you want to test (zesty, yakkety, xenial), and use the qemu autopkgtest backend with this image. Many of the tests require an isolation level that only a VM can provide.

All that means that you'd need to write a debian/tests/foo file (and update debian/tests/control) to run this new CI test, but I think it could be pretty simple. It would just do a git diff/stat against master and make sure that d/changelog was touched. There are some complications though:

* This test should only ever run on PRs; it should not run when already on the master branch, since there will be no diff against master!

* There should be a way to explicitly skip this test on a PR. Some changes are too minor to be mentioned in the changelog, and that's fine, but I would like it to be an explicit bypass. Unfortunately, I haven't thought of a good way to do that that doesn't also leave cruft in our tree. That's been the hangup for me so far, but if you have any thoughts, let's discuss!

Revision history for this message
Barry Warsaw (barry) wrote :

@sil2100 - assigned to you since you mentioned you wanted to give this a try. Feel free to reassign back to me if you want.

Changed in ubuntu-image:
assignee: Barry Warsaw (barry) → Łukasz Zemczak (sil2100)
Changed in ubuntu-image:
status: Confirmed → In Progress
Revision history for this message
Łukasz Zemczak (sil2100) wrote :

Thinking about a way to skip this test, maybe you already thought about this: but maybe we could enable this by introducing some special tag in one of the commits that are part of the pull request? We anyway have to do a diff between master trunk and the current branch to check if the debian/changelog is present - we could check the git commit messages of all missing commits as well. If one forgets to push it with some of his/hers commit, it can always be forced with an empty commit with git commit --allow-empty and just adding the 'special tag'.

This is still not perfect as it would leave this message tag in the git commit history, but I don't think that's so bad. All other choices would involve poking github in some way, to maybe get all the comments from the PR and scanning those. But for this I'd have to check if github exports any simple API we could reach out to...

Revision history for this message
Łukasz Zemczak (sil2100) wrote :

Oh, and another question about the autopkgtest env this is being run on. In your comment you wrote that this requires a source package built - does this mean that when the autopkgtests on the github CI is being run, is the source stripped off the git pieces? I don't think we ship the .git elements with our source packages, right? This might make things harder. Will I have access to the branch's git pieces in an autopkgtest?

Revision history for this message
Barry Warsaw (barry) wrote :

I was thinking we could use a label on the PR, something like 'trivial' or 'no-changelog'. Then the CI test could query the GH API to see if the PR had that label and if so, it wouldn't require that change to be listed.

One other thing: it's usually helpful that PRs are linked to LP bugs, so that might be another good test to run. OTOH, some changes don't need an LP bug. We could probably handle both cases for a 'trivial' label. So if the 'trivial' label is added, then it would be okay to omit the d/changelog entry, or to have a d/changelog entry with no LP: bug link. Without a 'trivial' tag, a d/changelog entry with an LP: link would be required. (Bonus if the LP bug number is chased to ensure it's valid with an ubuntu-image upstream bugtask.)

The one case this wouldn't handle is a 'trivial' tag where we still want a d/changelog entry w/o an LP link, but that is missing from the PR. I think that's fine; we'll get a false-ok on the CI test, but we can request the d/changelog entry in a PR review.

Revision history for this message
Łukasz Zemczak (sil2100) wrote :

Ah, ok, found an e-mail from you where you mention briefly how the autopkgtests are called for PRs. So git commands should be cool. Nevermind my last comment then!

Revision history for this message
Łukasz Zemczak (sil2100) wrote :

Looking into the github API in-between stuff then!

Barry Warsaw (barry)
Changed in ubuntu-image:
status: In Progress → Fix Committed
Barry Warsaw (barry)
Changed in ubuntu-image:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.