[github] Don't consider PR body in CI skip logic

[usmonster]

GitHub pull request edits weren't triggering CI when the PR body/description contained the [ci skip]/[skip ci] pattern. This was an issue, for example, in Dependabot-generated PRs that include commit messages from upstream dependencies that do include the pattern. Furthermore, no other CI service or vendor considers the PR body for the "ci skip" mechanism. This corrects the behavior so that only commit messages or PR titles are considered when a PR is edited.

New Pull Request Checklist

• [x] Run go fmt on your files (e.g. go fmt ./service/common.go, or on the whole service folder: go fmt ./service/...)
• [x] Write tests for your code
  • The tests should cover both "success" and "error" cases.
  • The tests should also check all the returned variables, don't ignore any returned value!
  • Ideally the tests should be easily readable, we usually use tests to document our code instead of code comments. An example, if you'd write a comment like "Given X this function will return Y" or "Beware, if the input is X this function will return Y" then you should implement this as a unit test, instead of writing it as a comment.
• ~[ ] If your Pull Request is more than a bug fix you should also check README.md and change/add the descriptions there - also feel free to add yourself as a contributor if you implement support for a new service ;)~
• [x] Before creating the Pull Request you should also run bitrise run test with the Bitrise CLI, to perform all the automatic checks (which will run on your Pull Request when you open it).

Summary of Pull Request

This Pull Request fixes the GitHub webhook logic so that "ci skip" in pull request bodies has no effect on whether or not CI is triggered, aligning it with other CI/git service implementations. Please note that CI will still be skipped as expected if the keyphrase appears in the PR title or in any part of a commit message. (Corrects logic introduced in #22.)

[usmonster]

Hello @sylank @viktorbenei ! Sorry for the direct pings, but just wanted to know the recommended way to get eyes on this. 😅 Thanks!

[viktorbenei]

Hey @usmonster 👋

Thanks for the ping! We started to discuss about this change with the team internally, but given this is a breaking change it needs a bit more preparation. Thanks for providing all the necessary information, it's a huge help! :)

We'll get back to you as soon as we have a plan.

In the meantime if this is a blocking issue for you a possible solution might be to run your own version of this hooks service - you can find instructions about how you can deploy it to your own Heroku account here https://github.com/bitrise-io/bitrise-webhooks#deploy-to-heroku

[usmonster]

Thanks for the response, @viktorbenei!

It's not blocking me at the moment (my workaround is to manually edit the PR description), though I'm sure many people run into this unexpected behavior (on certain Dependabot pull requests, for example) and spend a lot of time troubleshooting why there's no build result.

Please don't hesitate to ask if there's anything you'd need from me to help with preparations.

[usmonster]

Hello again! Just wondering if there's any progress on a plan for this. Thanks!

[viktorbenei]

Hi @usmonster - it's still under review. As it's a breaking change it needs more time for the preparation. It's not yet decided how we'll handle the breaking change for those who depend on how it works today.

[usmonster]

Thanks for the response @viktorbenei! I wonder if a blog post and documentation updates would be enough, or if you might want a deprecation notice first somewhere (in the activities section of the site) for these specific cases? Just some suggestions, but I'm looking forward to your decision. 🙂

[viktorbenei]

@usmonster indeed those are great ideas and things we're exploring, but also we'd gather data how many people this change would affect. Can't promise a release date yet as we have other things we have to finish first before we could dig into this one :)

[usmonster]

Hello @viktorbenei! I'm just checking up on this one—have you been able to measure the potential impact of this change or at least plan to look into it? (I wonder if it seems like a bigger change than it really is?) Please let me know if you have any questions or if I can do anything else to get this merged, and sorry for being so persistent. :) Thanks again!

[vturda]

Hey @usmonster, we checked your pull request again and it wouldn't solve your issue because you modified the code only when a pull request edit occurs. Skip logic is implemented here: https://github.com/bitrise-io/bitrise-webhooks/blob/master/service/hook/endpoint.go#L220

Anyway as @viktorbenei mentioned it would be a breaking change and need to be implemented for other providers as well e.g. GitLab, Bitbucket to keep consistency.

Today we've added some logging to measure how many of our users would be affected by this change. We need a few weeks to gather enough data, after that we're coming back to you.

Thank you for your patience!

[usmonster]

Thanks for the follow up! 😄

Hey @usmonster, we checked your pull request again and it wouldn't solve your issue because you modified the code only when a pull request edit occurs. Skip logic is implemented here: https://github.com/bitrise-io/bitrise-webhooks/blob/master/service/hook/endpoint.go#L220

My PR is explicitly focused on the PR edit, since it seems that's the only place with this unexpected behavior (I believe this covers PR creation as well, but I haven't looked at the code in a while). The line of code you point to seems to be focused on commit messages only, but please clarify if I've missed something.

Anyway as @viktorbenei mentioned it would be a breaking change and need to be implemented for other providers as well e.g. GitLab, Bitbucket to keep consistency.

It's a good point—I might've done this for other providers as well, but I only use GitHub at the moment.

Today we've added some logging to measure how many of our users would be affected by this change. We need a few weeks to gather enough data, after that we're coming back to you.

Thank you for your patience!

Yes, I see #117—thanks again, and I look forward to seeing this approved and implemented everywhere.

[vturda]

Unfortunately it does not cover PR creation, only PR title or description change.

Yeah, the commitMessage variable name can be confusing there but actually it's built from PR title + PR description in case of a pull or merge request.

[usmonster]

Today we've added some logging to measure how many of our users would be affected by this change. We need a few weeks to gather enough data, after that we're coming back to you.

Hello @vturda! Just getting back to you since it's been a few weeks. 😅 Is there any news? Thanks again!