Merge ~andersson123/autopkgtest-cloud/+git/autopkgtest-cloud-2:add_yamllint_to_pipeline into autopkgtest-cloud:master

[Tim Andersson]

ci: add yamllint to pipeline

ci: add yamllint to pipeline

[Paride Legovini (paride)]

The diff is clearly ok :-)

But the commit message is not very helpful. The conventional wisdom is that commit messages should explain the "what and why" of a change (while normally the "how" can be left out). I see the "what" in the commit message, but not the why, which only became clear to me from another comment of yours [0].

I think a commit message like:

lpci: add missing yamllint dependency to lint_test pipeline

is enough to make everything clear. (Note that this truly wasn't obvious to me, as I'm not that familiar with the lint_test script!)

[0] https://code.launchpad.net/~andersson123/autopkgtest-cloud/+git/autopkgtest-cloud-2/+merge/443815/comments/1181790

review: Needs Fixing

[Tim Andersson (andersson123)]

> The diff is clearly ok :-)
>
> But the commit message is not very helpful. The conventional wisdom is that
> commit messages should explain the "what and why" of a change (while normally
> the "how" can be left out). I see the "what" in the commit message, but not
> the why, which only became clear to me from another comment of yours [0].
>
> I think a commit message like:
>
> lpci: add missing yamllint dependency to lint_test pipeline
>
> is enough to make everything clear. (Note that this truly wasn't obvious to
> me, as I'm not that familiar with the lint_test script!)
>
> [0] https://code.launchpad.net/~andersson123/autopkgtest-
> cloud/+git/autopkgtest-cloud-2/+merge/443815/comments/1181790

Amended :~)

[Brian Murray (brian-murray)]

@paride - I noticed you used "lpci: " in your example commit message. Do you think we should standardize on something for commit messages? I saw you also used "cloud charm: " once.

[Tim Andersson (andersson123)]

Hmmm, yeah I think we should, from now on whenever I do an MP with lpci changes I'll pre-pend it with this

[Paride Legovini (paride)]

@Brian @Tim: I personally like to add some information on the scope of the change in the commit subject when it makes sense, especially when repositories cover different things/project. When it clearly makes sense I try to do it.

On adopting a team standard: I'm in favor of trying! We could try standardizing on a subset of:

  https://www.conventionalcommits.org/en/v1.0.0/

In particular we could start commit types, e.g.:

  ci: (for ci changes, superseding my "lpci:")
  fix: (when we unbreak stuff)
  feat: (new stuff)
  chore: (for e.g. updating armhf worker addresses)
  ...

The risks I see in doing so are:

- Us losing time / bikeshedding (not that big as a risk I think);
- Having too many changes that do not really fit into one of those commit types.

However we could try adopting this and see how it goes.

[Tim Andersson (andersson123)]

I'll start adding info a couple lines after main commit message on my commits from now on!

I'm in favour of adopting standards too. At a previous place I worked we had similar standards of 'fix:' 'ci:' 'chore:' etc. It makes browsing the git history much more pleasant.

There could definitely be some changes that don't fit into those categories, but I think 'chore' would encompass a lot. I think it'll be a good idea, especially as the team grows

I amended the commit message btw

[Paride Legovini (paride)]

The ubuntu-pro-client and cloud-init projects already do this in a loose way [0,1]. In particular instead of being strict on the

  type(scope): description

format, they either go with:

  type: description
  scope: description

whatever makes more sense. I find this works fine. Being strict on the conventionalcommits spec is useful when auto-generating changelogs / release notes for software releases, which we (Ubuntu QA) do not need to do much.

[0] https://github.com/canonical/ubuntu-pro-client/commits/main
[1] https://github.com/canonical/cloud-init/commits/main

[Tim Andersson (andersson123)]

Sounds good, happy to adopt this going forward!

[Paride Legovini (paride)]

Great, let's sync on this with Brian at standup.

Note that this specific MP is merged already, so no need to update anything for it. This is in main:

commit 3c301e50e6bd922f25cd9cafccc5e449d1a6bfae
Author: Tim Andersson <email address hidden>
Date: Mon Jun 5 13:57:26 2023 +0100

    add yamllint to pipeline for linting test for lpci

diff --git a/.launchpad.yaml b/.launchpad.yaml
index 57582c7..6dca924 100755
--- a/.launchpad.yaml
+++ b/.launchpad.yaml
@@ -5,5 +5,5 @@ jobs:
   lint_test:
     series: focal
     architectures: amd64
-    packages: [pylint, python3, shellcheck]
+    packages: [pylint, python3, shellcheck, yamllint]
     run: ./ci/lint_test