search

hyperledger-cacti / cacti

Hyperledger Cacti is a new approach to the blockchain interoperability problem
https://wiki.hyperledger.org/display/cactus
Apache License 2.0
344 stars 286 forks source link

ci(github): commit parity check to use PR title+description #3526

Closed petermetz closed 2 months ago

petermetz commented 2 months ago

Description

Right now the commit subject line is included in the parity check but not the PR title. The way GitHub generates the PR title is that it takes the subject line of the first commit and uses that as the title. The rest of the commit (the commit body) is used for the PR description.

Right now the parity check compares $PR_DESCRIPTION vs. COMMIT_MESSAGE(SUBJECT+BODY). The comparison should be two-fold instead:

  1. $PR_DESCRIPTION vs. COMMIT_MESSAGE_BODY and
  2. $PR_TITLE vs. COMMIT_MESSAGE_SUBJECT

As an example/reference see the pull request Peter has just submitted where the parity check fails like this:

The edge case being that the commit message body is empty (because release commits are self-explanatory anyway) and so the similarity index of PR description vs commit message body+subject gets over the threshold since the PR description is also empty (correctly mimicking the empty commit message body).

PR Link: https://github.com/hyperledger/cacti/pull/3525

Screenshot: Screenshot from 2024-09-08 12-32-08

cc: @jagpreetsinghsasan

Acceptance Criteria

  1. The commit check passes for pull requests such as the one on the screenshot.
  2. All previous valid pull requests are still passing after the fix
  3. No new false positives are introduced
  4. No new false negatives are introduced
jagpreetsinghsasan commented 2 months ago

Increased the scope of this task to also include the fix mentioned here: https://github.com/hyperledger/cacti/pull/3529#pullrequestreview-2291267457

jagpreetsinghsasan commented 2 months ago

@petermetz for this, shall we even check if the PR title contains chore(release) because if we straightaway check for PR title to match with commit title (incase of empty PR, commit body) then this could be exploited to skip the PR-Commit parity check altogether. So in my opinion, it would be better if such a check is only exclusive to a release.

petermetz commented 2 months ago

@jagpreetsinghsasan What I meant is that we would check both in a way that they are both necessary to match by a certain percentage of string similarity. So you couldn't hack it by providing matching titles but not matching bodies because the outcome would be (pseudocoded) isSuccessful = titleCheckOk && bodyCheckOk;

If this is not what your question was about then please provide a specific example so I can understand better.