confidential-containers / cloud-api-adaptor

Ability to create Kata pods using cloud provider APIs aka the peer-pods approach
Apache License 2.0
46 stars 76 forks source link

Consider updating the commit message check #1287

Closed stevenhorsman closed 1 year ago

stevenhorsman commented 1 year ago

The commit message action was originally written to match the confidential containers community guidelines about how to format commits. However it seems like we are the only repo that is enforcing this. I've created https://github.com/confidential-containers/confidential-containers/issues/143 to request the TSC to review this, but in the short-term we could have a discussion about how we might like to improve things.

I will try and capture a mixture of my own opinions and some ideas shared on the community call, but please keep me honest and share feedback in the comments here!

Having a commit format in general I think that having a format, even if it's just for cloud-api-adaptor and not project wide is useful for consistency and that our overall aims in commit messages should be:

As such I think having a rule that checks that there is a short-ish commit message header/summary (50 chars seems to be the general consensus) and that there is a commit message body are useful, but realistically I don't think we can come up with automation (until AI takes over) that can assess this, so human review is still required here

Subsystem Currently the rules are that the commit message summary must start with a single word that describes the area of code that the change applies too as a 'hint to the reader'.

We could discuss:

  1. Should we create a list of suggested subsystems for peer pods to try and make this more consistent?
  2. Should we make it optional/get rid of the guidelines at all
  3. Something else

Issue link requirement Currently rule is that every PR must have a commit that links to an issue that it fixes. Whilst this may be helpful for providing extra context (though ideally we'd want the commit message to have a summary of it) and links to discussion, for small changes like typos, or version bumps it can feel a bit onerous. My other observation (probably driven by my own laziness) is that when I am only creating issues at the time of raising the PR they tend to be fairly short and not give more value that the commit messages, so if we enforce it at that point then we get a reduction is the value it brings.

We could discuss:

  1. Removing the enforcement and making the fixes link optional and human review
  2. Creating an action that doesn't block merging, but adds a PR comment to suggest that an issue is created/PR description is added
  3. Add the size of the PR change into the above rules eg use something like https://github.com/apps/pull-request-size and say that issues are required/prompted for if the change is bigger than 9 lines?
  4. Something else
katexochen commented 1 year ago

Just leaving multiple short comments so people can upvote

As such I think having a rule that checks that there is a short-ish commit message header/summary (50 chars seems to be the general consensus) and that there is a commit message body are useful

I agree that this is useful, and I would keep it as it is, as long as there isn't a community wide rule (which I think should match what you describe here).

katexochen commented 1 year ago

Currently the rules are that the commit message summary must start with a single word that describes the area of code that the change applies too as a 'hint to the reader'.

I really like this practice, but I find it hard to enforce. Especially, I don't see how we could apply such a rule project wide. So I would vote for this to be optional in the future. I think if people who frequently contribute to the project do this often, people will follow our example automatically in many cases (like I did).

katexochen commented 1 year ago

My other observation (probably driven by my own laziness) is that when I am only creating issues at the time of raising the PR they tend to be fairly short and not give more value that the commit messages, so if we enforce it at that point then we get a reduction is the value it brings.

I agree with this, opening an issue when opening a PR is simply to late and misses the intent of an issue. IMO we should just remove this check.

stevenhorsman commented 1 year ago

At the CoCo community meeting on 31st Aug it was clear that despite the guidelines there is widespread agreement that the CoCo guidelines (and especially the Fixes requirement) shouldn't be binding, so we should feel free to update this