chdsbd / kodiak

🔮 A bot to automatically update and merge GitHub PRs
https://kodiakhq.com
GNU Affero General Public License v3.0
1.03k stars 65 forks source link

Kodiak not stripping HTML comments from PR messages #800

Closed jpalmieri closed 2 years ago

jpalmieri commented 2 years ago

We're seeing html comments stay in the merge messages when merging PRs, despite our configuration matching the recipe for Better Merge Messages in the docs.

Let me know if there's other information that I could provide which might be relevant to this.

chdsbd commented 2 years ago

Can you provide an example of the Kodiak configuration and the pull request body?

jpalmieri commented 2 years ago

I'd rather not provide the entire config, but here's the section that seems relevant. If there are other parts that might be relevant, let me know and I can post those.

[merge.message]
# by default, github uses the first commit title for the PR of a merge.
# "pull_request_title" uses the PR title.
title = "pull_request_title" # default: "github_default", options: "github_default", "pull_request_title"

# by default, GithHub combines the titles a PR's commits to create the body
# text of a merge. "pull_request_body" uses the content of the PR to generate
# the body content while "empty" simple gives an empty string.
body = "pull_request_body" # default: "github_default", options: "github_default", "pull_request_body", "empty"

# GitHub adds the PR number to the title of merges created through the UI.
# This setting replicates that feature.
include_pr_number = true # default: true

# markdown is the normal format for GitHub merges
body_type = "markdown" # default: "markdown", options: "plain_text", "markdown", "html"

# useful for stripping HTML comments created by PR templates when the `markdown` `body_type` is used.
strip_html_comments = true # default: false

I'd rather not send over an actual commit message, but here's an example of what we're seeing

Add cool new feature (#123)

### :label: Jira ticket

<!-- Add the Jira ticket corresponding to this Pull request -->

https://company.atlassian.net/browse/COOL-123

### :loudspeaker: Type of change

<!--- Remove what's irrelevant -->

- New feature (non-breaking change which adds functionality)
### :scroll: Description

<!--
   Describe your changes in detail.
   Mainly answer the "What" and "How". What is the PR changing? What is it adding/modifying/removing? What will the new behavior be?
   How is the change introduced?
-->

Add feature which is really cool and useful.

...
chdsbd commented 2 years ago

@jpalmieri Thanks! That's enough to replicate the issue on my side. I'll look into this.

Oddly Kodiak did remove the HTML comment in this example PR I used earlier:

Bumps [serialize-error](https://github.com/sindresorhus/serialize-error) from 8.1.0 to 9.1.1.
- [Release    notes](https://github.com/sindresorhus/serialize-error/releases)
- [Commits](https://github.com/sindresorhus/serialize-error/compare/v8.1.0...v9.1.1)

<!-- testing123 -->
---
updated-dependencies:
- dependency-name: serialize-error
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
chdsbd commented 2 years ago

@jpalmieri Do you have your merge method set to "rebase"? In my testing just now, it seems that Kodiak won't be able to set the commit message for "rebase" merge types, because there isn't a new commit being created like there is with "merge" or "squash".

Maybe we could update Kodiak to error for this kind of invalid configuration, but I'm hesitant to interrupt existing Kodiak installations

We should definitely update the docs though.

jpalmieri commented 2 years ago

@chdsbd Our merge method is set to squash

[merge]

...

# choose a merge method. If the configured merge method is disabled for a
# repository, Kodiak will report an error in a status message.
method = "squash" # default: "merge", options: "merge", "squash", "rebase"

...
chdsbd commented 2 years ago

@jpalmieri interesting. Do you mind sending me your full Kodiak config. You can email me at chris@kodiakhq.com if you'd like to share it privately.

Thanks

jpalmieri commented 2 years ago

@chdsbd Just sent. Thanks!

chdsbd commented 2 years ago

@jpalmieri Thanks for your patience. I've done some testing and found the bug exists in https://github.com/chdsbd/markdown-html-finder. It seems that removing the emoji from the PR template allows Kodiak to strip the html comments correctly.

I'm going to continue investigating why markdown-html-finder is having issues

chdsbd commented 2 years ago

@jpalmieri I have a PR that should resolve this issue: #805

chdsbd commented 2 years ago

@jpalmieri I've deployed #805, which should resolve the issue. Let me know if you have any more issues.