terraform-aws-modules / terraform-aws-lambda

Terraform module, which takes care of a lot of AWS Lambda/serverless tasks (build dependencies, packages, updates, deployments) in countless combinations 🇺🇦
https://registry.terraform.io/modules/terraform-aws-modules/lambda/aws
Apache License 2.0
885 stars 658 forks source link

fix(dev): Add pre-commit hook for semantic commit message check #556

Closed pdecat closed 2 months ago

pdecat commented 2 months ago

This PR adds a pre-commit check using https://github.com/compilerla/conventional-pre-commit to ensure commit messages are valid.

Example usage:

# git commit -m "Fix AttributeError os.exists is not valid python attribute." --allow-empty
Terraform fmt........................................(no files to check)Skipped
Terraform wrapper with for_each in module............(no files to check)Skipped
Terraform docs.......................................(no files to check)Skipped
Terraform validate with tflint.......................(no files to check)Skipped
Terraform validate...................................(no files to check)Skipped
check for merge conflicts............................(no files to check)Skipped
fix end of files.....................................(no files to check)Skipped
trim trailing whitespace.............................(no files to check)Skipped
Conventional Commit......................................................Failed
- hook id: conventional-pre-commit
- exit code: 1

[Bad Commit message] >> Fix AttributeError os.exists is not valid python attribute.

        Your commit message does not follow Conventional Commits formatting
        https://www.conventionalcommits.org/

        Conventional Commits start with one of the below types, followed by a colon,
        followed by the commit subject and an optional body seperated by a blank line:

            fix feat docs ci chore

        Example commit message adding a feature:

            feat: implement new API

        Example commit message fixing an issue:

            fix: remove infinite loop

        Example commit with scope in parentheses after the type for more context:

            fix(account): remove infinite loop

        Example commit with a body:

            fix: remove infinite loop

            Additional information on the issue caused by the infinite loop
pdecat commented 2 months ago

Marked as draft as the rules for PR title validation differ from semantic commit messages, and need alignment.

pdecat commented 2 months ago

The remaining issue has to do with the uppercase requirement is less common AFAIK:

Error: The subject "add pre-commit hook for semantic commit message check" found in the pull request title "fix(dev): add pre-commit hook for semantic commit message check"
didn't match the configured pattern. Please ensure that the subject
starts with an uppercase character.

https://github.com/terraform-aws-modules/terraform-aws-lambda/actions/runs/8522151845/job/23341728163?pr=556

But at least, it is the same subjectPattern across all modules: https://github.com/search?q=org%3Aterraform-aws-modules+action-semantic-pull-request+subjectPattern&type=code

pdecat commented 2 months ago

Sadly, conventional-pre-commit does not seem to allow enforcing the same subject pattern rule, it accepts subjects with both lowercase and uppercase first letter.

pdecat commented 2 months ago

Hi @antonbabenko, do you think it would be acceptable to relax that PR title rule a bit to allow both upper and lower case first letter? I understand that means quite some work as it would require submitting the same change to all repositories under terraform-aws-modules to keep things consistent.

Or should we try and submit a PR to conventional-pre-commit to implement such additional rules?

FWIW: all semantic commit message examples at https://www.conventionalcommits.org/ use a lowercase first letter.

antonbabenko commented 2 months ago

I don't want us to change what has been (or "was") working for a long time. The PR title should be written well (I edit it now), but the commits itself can be anything.

antonbabenko commented 2 months ago

Let's close this PR.

pdecat commented 2 months ago

At least, we could keep the basic checks to avoid issues like we faced last week: https://github.com/terraform-aws-modules/terraform-aws-lambda/pull/554#issuecomment-2027330956

pdecat commented 2 months ago

but the commits itself can be anything

it is the commit message that triggers the semantic release behavior

pdecat commented 2 months ago

I guess using the PR title as the commit message only works when squashing during PR merge, but it does not work when there's a single commit AFAIU.

pdecat commented 2 months ago

Apparently, there's something that can be done to avoid last week's issue:

Legacy configuration for validating single commits

When using "Squash and merge" on a PR with only one commit, GitHub will suggest using that commit message instead of the PR title for the merge commit. As it's easy to commit this by mistake this action supports two configuration options to provide additional validation for this case.

# If the PR only contains a single commit, the action will validate that
# it matches the configured pattern.
validateSingleCommit: true
# Related to `validateSingleCommit` you can opt-in to validate that the PR
# title matches a single commit to avoid confusion.
validateSingleCommitMatchesPrTitle: true

However, GitHub has introduced an option to streamline this behaviour, so using that instead should be preferred.

https://github.com/amannn/action-semantic-pull-request?tab=readme-ov-file#legacy-configuration-for-validating-single-commits

antonbabenko commented 2 months ago

I think I clicked the wrong button last week when merging PR using the GitHub Mobile app. I have just reviewed the settings for this repository to make sure that we always "Squash and merge". Thanks for the follow-up!

github-actions[bot] commented 1 month ago

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.