Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
petermetz opened this issue Sep 8, 2024 · 3 comments · Fixed by #3542
Closed

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

petermetz opened this issue Sep 8, 2024 · 3 comments · Fixed by #3542
Assignees
@jagpreetsinghsasan
Labels
Flaky-Test-Automation Issues related to test stability (which is a long running issue that can never fully be solved) P4 Priority 4: Low Tests Anything related to tests be that automatic or manual, integration or unit, etc.

Comments

@petermetz
Copy link
Contributor

petermetz commented Sep 8, 2024 •
edited
Loading

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: #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
@petermetz petermetz added Flaky-Test-Automation Issues related to test stability (which is a long running issue that can never fully be solved) Tests Anything related to tests be that automatic or manual, integration or unit, etc. P4 Priority 4: Low labels Sep 8, 2024
@jagpreetsinghsasan jagpreetsinghsasan self-assigned this Sep 10, 2024
@jagpreetsinghsasan jagpreetsinghsasan moved this from Todo to In Progress in Cacti_Scrum_Project_v2_Release Sep 11, 2024
@jagpreetsinghsasan
Copy link
Contributor

Increased the scope of this task to also include the fix mentioned here: #3529 (review)

@jagpreetsinghsasan
Copy link
Contributor

@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
Copy link
Contributor Author

@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.

jagpreetsinghsasan pushed a commit to jagpreetsinghsasan/cactus that referenced this issue Sep 16, 2024
ci(github): commit parity to use PR title in check
b0c707b 
    Primary Changes
    ---------------
    1. Updated the script to include the check for releases
    2. Fixed certain regex and added a new regex for issue/PR
       references. This is done because the issue numbers tagged in
       PR message or commit messages are sometime resolved directly
       and sometimes parsed with the orgname.
    3. With the new regex in 2), we can now safely check for parity
       while including the fixes/depends line, further loosing the parity
       check, thus reducing false-positives

Fixes hyperledger-cacti#3526

Signed-off-by: jagpreetsinghsasan <[email protected]>
@jagpreetsinghsasan jagpreetsinghsasan mentioned this issue Sep 16, 2024
Merged
5 tasks
jagpreetsinghsasan pushed a commit to jagpreetsinghsasan/cactus that referenced this issue Sep 16, 2024
ci(github): commit parity to use PR title in check
b4f43ad 
    Primary Changes
    ---------------
    1. Updated the script to include the check for releases
    2. Fixed certain regex and added a new regex for issue/PR
       references. This is done because the issue numbers tagged in
       PR message or commit messages are sometime resolved directly
       and sometimes parsed with the orgname.
    3. With the new regex in 2), we can now safely check for parity
       while including the fixes/depends line, further loosing the parity
       check, thus reducing false-positives

Fixes hyperledger-cacti#3526

Signed-off-by: jagpreetsinghsasan <[email protected]>
jagpreetsinghsasan pushed a commit to jagpreetsinghsasan/cactus that referenced this issue Sep 16, 2024
ci(github): commit parity to use PR title in check
24f2e06 
    Primary Changes
    ---------------
    1. Updated the script to include the check for releases
    2. Fixed certain regex and added a new regex for issue/PR
       references. This is done because the issue numbers tagged in
       PR message or commit messages are sometime resolved directly
       and sometimes parsed with the orgname.
    3. With the new regex in 2), we can now safely check for parity
       while including the fixes/depends line, further loosing the parity
       check, thus reducing false-positives

Fixes hyperledger-cacti#3526

Signed-off-by: jagpreetsinghsasan <[email protected]>
petermetz pushed a commit to jagpreetsinghsasan/cactus that referenced this issue Sep 17, 2024
@petermetz
ci(github): commit parity to use PR title in check
e9b8682 
    Primary Changes
    ---------------
    1. Updated the script to include the check for releases
    2. Fixed certain regex and added a new regex for issue/PR
       references. This is done because the issue numbers tagged in
       PR message or commit messages are sometime resolved directly
       and sometimes parsed with the orgname.
    3. With the new regex in 2), we can now safely check for parity
       while including the fixes/depends line, further loosing the parity
       check, thus reducing false-positives

Fixes hyperledger-cacti#3526

Signed-off-by: jagpreetsinghsasan <[email protected]>
petermetz pushed a commit that referenced this issue Sep 17, 2024
@petermetz
ci(github): commit parity to use PR title in check
b64ac67 
    Primary Changes
    ---------------
    1. Updated the script to include the check for releases
    2. Fixed certain regex and added a new regex for issue/PR
       references. This is done because the issue numbers tagged in
       PR message or commit messages are sometime resolved directly
       and sometimes parsed with the orgname.
    3. With the new regex in 2), we can now safely check for parity
       while including the fixes/depends line, further loosing the parity
       check, thus reducing false-positives

Fixes #3526

Signed-off-by: jagpreetsinghsasan <[email protected]>
@github-project-automation github-project-automation bot moved this from In Progress to Done in Cacti_Scrum_Project_v2_Release Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jagpreetsinghsasan jagpreetsinghsasan

Labels
Flaky-Test-Automation Issues related to test stability (which is a long running issue that can never fully be solved) P4 Priority 4: Low Tests Anything related to tests be that automatic or manual, integration or unit, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

ci(github): commit parity to use PR title in check jagpreetsinghsasan/cactus
2 participants
@petermetz @jagpreetsinghsasan