- Issue created by @geek-merlin
- πΊπΈUnited States dww
- πΊπΈUnited States dww
Slightly off topic, but not sure where else to post this. I utterly hate the default commit messages that core uses and that the d.o issue UI suggests. π’ I've been on a personal campaign to try to change it for years.
- π± Discuss if git commit messages should be multiple lines Needs review
- π± [policy, no patch] Determine format for commit credit for individuals/organizations/customers Needs review
- https://drupal.slack.com/archives/C02LJCF78E8/p1706746729642249
- https://drupal.slack.com/archives/C1BMUQ9U6/p1706612626929429
So for contribs I (co)maintain, I do it how I think it should be done:
https://git.drupalcode.org/project/inline_entity_form/-/commit/c753ec180...
commit c753ec180166fd9a12af9d9d047730d2ee2650b6 Author: Derek Wright <git@dwwright.net> Date: Wed Jan 31 14:56:38 2024 -1000 task: #3149783: Remove hardcoded word 'entities' in EntityInlineForm::getEntityTypeLabels() Authored-by: globexplorer (Paul Mrvik) <45455-globexplorer@users.noreply.drupalcode.org> Authored-by: viren18febS (Virendra Singh) <64354-viren18febS@users.noreply.drupalcode.org> Authored-by: dww (Derek Wright) <git@dwwright.net> Reviewed-by: dcam (David Cameron) <9862-dcam@users.noreply.drupalcode.org> Reviewed-by: joachim (Joachim Noreiko) <20313-joachim@users.noreply.drupalcode.org>
I'd love it if we adopted this project-wide. The problem is that the automated tools don't make it easy, and it's more work to manually format these like this. In Slack, @larowlan suggested adding a CI "build" task that tries to query the issue and construct a better commit message and have that available as an "artifact" we can copy/paste when merging. I'll probably explore that option and see how it goes. If it works, I'll try to contribute it as a patch to the upstream gitlab testing templates, so all projects will start doing it (or at least allow easy opt-in or something).
- πΊπΈUnited States dww
p.s. https://www.conventionalcommits.org/en/v1.0.0/ for reference / interested readers.
- πΊπΈUnited States dcam
RE commit messages:
I don't have any strong feelings either way. Although now that I think on it I'll readily acknowledge that the D.o style messages can be very unfriendly since they lump all the usernames between the issue number and description. That's never cool when you're looking at histories. So I get that there's room for improvement. You have my support for doing things differently. But I'll echo @larowlan's sentiment and say "the more automated, the better." - πΊπΈUnited States dcam
While we're discussing maintenance policies, I'd like to circle back around to what we were talking about in the latest comments of π Adopt GitlabCi Downport . To refresh everyone's memory:
Drupal CI tests may be disabled for branches that have implemented GitLab CI testing. Drupal CI tests may be selectively turned off only for certain branches. For instance, the 3.x tests may be disabled while leaving the 7.x tests on.
Pros:
- It will save the association some money on testing costs.
- We can enforce that changes must continue to pass PHPCS and PHPStan checks. We've all worked hard on getting those to pass lately, so let's not regress!
Cons (depending on your point of view):
- Existing patches must be converted to MRs if additional work has to be done. That may result in a lot of extra work for everyone.
- People who submit patches will need to be directed to submit an MR instead.
I think that there's another con I'm not remembering.
@geek-merlin said "Let's text some contribution rules in a new issue." This seems like the appropriate place.
- πΊπΈUnited States dww
Here's another maintenance policy question:
For an issue like π Opt-in to wider test coverage from templates via gitlab-ci.yml variables RTBC , where I proposed and wrote the change, am I "allowed" to commit that now that someone else independently RTBC'ed? Presumably @geek-merlin has committed plenty of their own patches over the years. π I don't want to be a bottleneck and say we must do this like core does and if a given person helped write it they must not be the one to commit / merge it. But I don't want to be a "cowboy" and go off doing my own thing without calibrating expectations.
In that specific case, since it's a test-only change and therefore impossible to be disruptive to production sites, I'd feel very comfortable committing it, especially since it was RTBC'ed by an acknowledged "issue maintainer" for this project.
But I'm waiting to do so to get clarity from @geek-merlin on expectations around this aspect going forward, now that I've got push access to the repo.
Thanks!
-Derek - πΊπΈUnited States dww
Re: MRs vs. patches, I'm all for leaving patch testing enabled as long as possible, but it is being phased out, so eventually folks are going to have to get used to contributing via MR. I don't think we need to spend too much time soul searching about it. The DA will decide for us in a matter of months when they finally pull the plug. We could selectively turn off configured tests before then. Looking at https://www.drupal.org/node/1181848/qa β , we've only got "on commit" and "issue testing" tests. We could probably remove most of the 8.x-1.x tests now.
I'd rather see the full gitlab-ci pipeline for a change before we merge, but worst case that we don't, we'll see a pipeline result as soon as we do merge it. We can either open a followup or revert as appropriate. I'd never commit something and tag a release before the branch pipeline was fully green again.
Anyway, maybe this topic needs its own dedicated issue so we can decide what to do and act on it in a more clear spot with focused scope. That might help with visibility, too.
Thanks,
-Derek - πΊπΈUnited States dcam
Announced today: Drupal CI is getting shut down on July 1 β .