cocogitto / cocogitto-bot

A pedantic conventional commit github bot powered by cocogitto
3 stars 1 forks source link

[BUG] Does not consult cog.toml configuration file in repository #9

Closed stephenc closed 11 months ago

stephenc commented 1 year ago

Describe the bug We have configured our cog.toml with ignore_merge_commits = true but the bot fails if there are merge commits in the pull request

To Reproduce Steps to reproduce the behavior:

  1. Create a repo with a cog.toml that has ignore_merge_commits = true
  2. Create a pull request with some commits that are valid conventional commits
  3. Push some more commits to the main branch such that the pull request needs updating
  4. Update the pull request my merging the changes from main, GitHub will typically create a commit with "Merge ..."
  5. The bot will mark the commit as not conventional despite the ignore_merge_commits = true setting

Expected behavior A clear and concise description of what you expected to happen.

When cog.toml has ignore_merge_commits = true then merge commits in the pull request should be ignored

Additional context Add any other context about the problem here.

oknozor commented 1 year ago

Yes the bot needs to be updated to read the cog.toml conffiguration. Currently it does not

the-wondersmith commented 1 year ago

@oknozor I started tackling this over the weekend actually, and am having trouble finding a convenient place in the bot's flow to insert a check for the existence of cog.toml in the target codebase and subsequent check of cog.toml's content if it exists.

Also I noticed this, which looks like any commit whose message begins with Merge pull request is supposed to be omitted from consideration entirely (by default), but that doesn't seem to be the real-world case from observed behavior. 🤔

Obviously respecting whatever is defined in a codebase's cog.toml would be the ideal behavior, but actually behaving as if ignore_merge_commits is true by default isn't a bad stop-gap (IMOFWIW).

If you'll point me in the right direction, I'll happily bang out the updated code. 😁

the-wondersmith commented 12 months ago

@oknozor I started tackling this over the weekend actually, and am having trouble finding a convenient place in the bot's flow to insert a check for the existence of cog.toml in the target codebase and subsequent check of cog.toml's content if it exists.

Also I noticed this, which looks like any commit whose message begins with Merge pull request is supposed to be omitted from consideration entirely (by default), but that doesn't seem to be the real-world case from observed behavior. 🤔

Obviously respecting whatever is defined in a codebase's cog.toml would be the ideal behavior, but actually behaving as if ignore_merge_commits is true by default isn't a bad stop-gap (IMOFWIW).

If you'll point me in the right direction, I'll happily bang out the updated code. 😁

@oknozor Shameless self bump

oknozor commented 12 months ago

hey @the-wondersmith, Here are the required step to get there:

  1. Get cog.toml using the Github API (see: https://docs.rs/octocrab/latest/octocrab/models/repos/struct.Content.html#method.decoded_content)
  2. Use cocogitto as a lib instead of conventional_commit_parser to be able to use verify instead of just parsing the commit message.

Hope that helps, let me know if you need anything!

the-wondersmith commented 11 months ago

@oknozor That does help a ton.

I've got a decent first pass worked out, but hit a minor snag - cocogitto needs to make ConventionalCommitError public so that the error field of CommitErrorReport can be updated to use it instead of ParseError from conventional_commit_parser.

Is that feasable?

oknozor commented 11 months ago

Sure we can do this, I think you should work with a local version of cocogitto and make both PR at the same time.

the-wondersmith commented 11 months ago

@oknozor PR's are up - cocogitto / cocogitto-bot

I've run the tests in cocogitto itself, so I'm confident in those changes. I'm also confident in the theory behind the changes in the bot codebase, but it doesn't have tests so I'm not 100% confident on the best way to guarantee everything.

Also, the Ignored variant of the bot's CommitReport enum ended up technically not being necessary, but it obviously exists for a reason so I've just marked it as #[allow(dead_code)] for the moment until you'd had a chance to render a verdict on keeping it or not.

Eager to hear your thoughts on both fronts 😁

oknozor commented 11 months ago

Kudos to @the-wondersmith :heart: :rocket: !

screenshot-2023-08-01T11:17:40