Opened 15 months ago

Last modified 14 months ago

#6380 new change

Have hg commit hook check Noissue commits have linked review URL

Reported by: kzar Assignee:
Priority: Unknown Milestone:
Module: Sitescripts Keywords:
Cc: fhd, sebastian, kvas, tlucas, greiner, sergz Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description

Background

It is considered best practice at eyeo to add a link to the codereview for Noissue commits in the commit message. For an example see this commit:

Noissue - Remove devbuild update URL for Firefox builds

Review: https://codereview.adblockplus.org/29526585/

Unfortunately this is extremely easy to forget, I often do. Why not have a Mercurial hook check this for us? (For some context about the existing hg hooks see #3674.)

What to change

Have the server reject Noissue commits without a codereview URL, or some text to indicate there was none. Some examples of valid commit messages:

Noissue - Some short message

Review: https://codereview.adblockplus.org/29526585/

or

Noissue - Some short message

Review: None

or

Noissue - Some short message

Review: None
This is a long mention to further clarify the commit's purpose.

Questions

  • What about commits produced by the release automation and other scripts? I guess we have to modify those to include "Review: None"
  • Do we all agree about the syntax? How strict do we want to be, for example allowing the "Review: " prefix to be omitted?
  • Does everyone agree about this hook being applied for all repositories, or should we have a whitelist?

Change History (5)

comment:1 Changed 15 months ago by kzar

  • Summary changed from Have hg commit hook check noissue commits have linked review URL to Have hg commit hook check Noissue commits have linked review URL

comment:2 Changed 15 months ago by sergz

  • Cc sergz added

comment:3 Changed 14 months ago by greiner

I agree that enforcement of rules we impose shouldn't harm in this case. However, note that such rules are allowed to differ between project teams but yeah, ideally, we would agree on certain rules across all projects as we do with our coding styles.

comment:4 follow-up: Changed 14 months ago by kvas

I don't think that it would be a good idea to push this change onto all users of hg.adblockplus.org. Making it optional, enabled on a per-repository basis, could be useful though.
As for the syntax, perhaps just checking for "Review:" somewhere in the commit message of any Noissue commit would be enough?

comment:5 in reply to: ↑ 4 Changed 14 months ago by sergz

Replying to kvas:

I don't think that it would be a good idea to push this change onto all users of hg.adblockplus.org. Making it optional, enabled on a per-repository basis, could be useful though.

IMO it definitely should be optional, however what about firstly enabling it everywhere and disable when something goes wrong, so we will know such special cases?

As for the syntax, perhaps just checking for "Review:" somewhere in the commit message of any Noissue commit would be enough?

Sounds legit.

Note: See TracTickets for help on using tickets.