fix: check_commit_msg used for PRs titles only

[kevholmes]

This resolves #258. When reviewing the original issue #205 it sounds like this was only supposed to run on pull_request events. My fix will ensure that this only runs when contributors submit a PR to the datree repo.

[romanlab]

@kevholmes Actually, it would be great to give the contributor an indication before submitting a PR. It's ok that it runs on a push event, it just needs to ignore the main branch. If we do it this way, we can then refactor the script and use it with pre-commit.

maybe you can use

on: 
  push:
    branches-ignore:
      - 'main'

to achieve that

WDYT?

[kevholmes]

@romanlab I like the idea of adding the python script as a local pre-commit hook, that way the contributor could see the issue before they ever make a commit.

I'd say it's a pretty minute difference between push and pull_request really. If I fork datree's GH repo and push commits directly to my feature branch the push trigger might run (as long as I have manually gone in and enabled Actions for my forked repo.) So it's not exactly automated for end users. I like reducing the scope of when this runs to something more narrow like pull_request as a good practice as the blanket on push could overlap with other git repo work you're doing that might not need to meet that Check's criteria and cause an unnecessary failure.

Also by default with on: pull_request that includes synchronize events (which are individual commits to a PR) so if a user makes a PR to this repo - but the check_commit_msg check fails - they can easily update the name of the commit in the GitHub UI to the correct (passing) format - and the check should re-run. If we included the edited event under a pull_request I believe we could catch the user updating the PR meta info itself and re-run the check - which I do not believe would be triggered on a push event since since that's outside of editing GH PRs (and their names.)

[romanlab]

@kevholmes That's a valid point. But looking at the event structure for a pull_request event, i didn't see a commits key and not sure the script will work for the event. You can see the details here

[kevholmes]

@kevholmes That's a valid point. But looking at the event structure for a pull_request event, i didn't see a commits key and not sure the script will work for the event. You can see the details here

pull_request is a little confusing - they call the events synchronize when pushing follow-up commits to a PR. I think because of how GH handles PRs behind the scenes.

[romanlab]

@kevholmes That's a valid point. But looking at the event structure for a pull_request event, i didn't see a commits key and not sure the script will work for the event. You can see the details here

pull_request is a little confusing - they call the events synchronize when pushing follow-up commits to a PR. I think because of how GH handles PRs behind the scenes.

That's fine, i see the synchronize action but the event structure doesn't have a commits key. I approved the workflow, you can see the check failing

[kevholmes]

Ahh ok, I see what you're saying @romanlab regarding the python script possibly not working with this approach. It seems like we really want to run something that examines the GH PR's title and gives feedback regarding if they meet the standard or not. Each commit to a PR might not follow the approach, but ultimately its the PR title that will get squashed-and-merged into main and should follow the spec.

[kevholmes]

Maybe using something like this to review the PR titles?

[romanlab]

Maybe using something like this to review the PR titles?

We prefer to not have external dependencies on projects with almost no community. pr-title-checker has only 31 stars. It'll be easier to adjust the script to validate the pr title.

[kevholmes]

@romanlab How about this? I've refactored the python script and workflow to handle PR title checking instead of commit message checking. This has a more narrow scope and I think it meets the team's criteria. I have added some additional error handling to the python script as I noticed issues with errors when attempting to parse a : that might not be in the message if the author forgot it. I also added some additional verbosity to help explain to contributors why the check has failed (ie the message needs to be under 72 characters.)

I used some sample output from a test PR in my fork of datree's repo to test the Python script, here's the sample json. I am able to edit the PR title and verify the check will re-run at that time, and correctly passes or then fails depending on what I changed my PR title to.

[kevholmes]

I've got some sample code running in my public fork if you'd like to review it there: kevholmes#2