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.

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

ci(github): commit parity check for PRs should show what is wrong #3470

Closed
petermetz opened this issue Aug 9, 2024 · 5 comments · Fixed by #3504
Closed

ci(github): commit parity check for PRs should show what is wrong #3470

petermetz opened this issue Aug 9, 2024 · 5 comments · Fixed by #3504
Assignees
Labels
bug Something isn't working Hacktoberfest Hacktoberfest participants are welcome to take a stab at issues marked with this label. P3 Priority 3: Medium Tests Anything related to tests be that automatic or manual, integration or unit, etc.
Milestone

Comments

@petermetz
Copy link
Contributor

Description

I dogfooded the PR commit message parity check and could not figure out what the problem was (e.g. what was different in my PR description vs my commit message).

The pull request where the check is failing despite my best effort to sync up the description and the commit message:
https://github.com/hyperledger/cacti/pull/3456

Acceptance Criteria

  1. The custom check should show what's exactly different between the two texts that its comparing so that it's quickly obvious to the contributor what they need to change and where.
  2. The nicest would be to show a diff like this screenshot below (or something equivalent that can be rendered as text). With that said, anything that efficiently conveys the required information will do since our time and resources are limited and we cannot put images in the GitHub CI logs anyway.
    image
@petermetz petermetz added bug Something isn't working Hacktoberfest Hacktoberfest participants are welcome to take a stab at issues marked with this label. Tests Anything related to tests be that automatic or manual, integration or unit, etc. P3 Priority 3: Medium labels Aug 9, 2024
@petermetz petermetz added this to the v2.0.0 milestone Aug 9, 2024
@jagpreetsinghsasan jagpreetsinghsasan self-assigned this Aug 13, 2024
@jagpreetsinghsasan
Copy link
Contributor

I am working on this

@jagpreetsinghsasan
Copy link
Contributor

@petermetz I will make the checks looser while keeping the check relevant (It would be very irritating working on a task, creating a PR and then getting stuck comparing and fixing the PR and the related commit.)

@petermetz petermetz moved this from Todo to In Progress in Cacti_Scrum_Project_v2_Release Aug 13, 2024
@petermetz
Copy link
Contributor Author

@jagpreetsinghsasan Thank you for the work on the improvements in advance! I thought about it a little more and would strongly recommend to refactor it to be less strict by only demanding a certain level of string similarity, otherwise it will probably just end up being super annoying 90% of the time for contributors in the cases when they just have some silly typo making a difference.

I found this library that looks very versatile and perfect for the task:
https://www.npmjs.com/package/string-similarity-js

Specifically I would recommend the custom check to have a 90% (0.9) similarity ratio demand by default (so maybe they won't be the exact same but as long as at least they are mostly the same we can be OK with it as a best-of-both-worlds tradeoff.

The other thing: It would be very handy to be able to configure the threshold from an env variable so that later on if we find that a higher or lower threshold is better, we don't have to touch the code at all just set the env var in ci.yaml or somewhere else where the job is defined (technically still a code-change I know, but it's in the yaml not in the actual logic)

@jagpreetsinghsasan
Copy link
Contributor

Oh wow. This looks like a perfect solution to do a looser string matching. I will incorporate the same with a variable input of similarity ratio. Thanks @petermetz for the insights

@petermetz
Copy link
Contributor Author

@jagpreetsinghsasan You got it! Thank you as well!

jagpreetsinghsasan pushed a commit to jagpreetsinghsasan/cactus that referenced this issue Sep 1, 2024
    Primary Changes
    ---------------
    1. Added a commit-pr similarity ratio to lessen the check strictness
    2. Added an env var to the workflow to easily control the similarity
       ratio

    Changes required to incorporate 1)
    ---------------------------------
    3. Added string-similarity-js package in package.json

Fixes hyperledger-cacti#3470

Signed-off-by: jagpreetsinghsasan <[email protected]>
jagpreetsinghsasan pushed a commit to jagpreetsinghsasan/cactus that referenced this issue Sep 1, 2024
    Primary Changes
    ---------------
    1. Added a commit-pr similarity ratio to lessen the check strictness
    2. Added an env var to the workflow to easily control the similarity
       ratio

    Changes required to incorporate 1)
    ---------------------------------
    3. Added string-similarity-js package in package.json

Fixes hyperledger-cacti#3470

Signed-off-by: jagpreetsinghsasan <[email protected]>
jagpreetsinghsasan pushed a commit to jagpreetsinghsasan/cactus that referenced this issue Sep 2, 2024
    Primary Changes
    ---------------
    1. Added a commit-pr similarity ratio to lessen the check strictness
    2. Added an env var to the workflow to easily control the similarity
       ratio

    Changes required to incorporate 1)
    ---------------------------------
    3. Added string-similarity-js package in package.json

Fixes hyperledger-cacti#3470

Signed-off-by: jagpreetsinghsasan <[email protected]>
jagpreetsinghsasan pushed a commit to jagpreetsinghsasan/cactus that referenced this issue Sep 2, 2024
    Primary Changes
    ---------------
    1. Added a commit-pr similarity ratio to lessen the check strictness
    2. Added an env var to the workflow to easily control the similarity
       ratio

    Changes required to incorporate 1)
    ---------------------------------
    3. Added string-similarity-js package in package.json

Fixes hyperledger-cacti#3470

Signed-off-by: jagpreetsinghsasan <[email protected]>
jagpreetsinghsasan pushed a commit to jagpreetsinghsasan/cactus that referenced this issue Sep 2, 2024
    Primary Changes
    ---------------
    1. Added a commit-pr similarity ratio to lessen the check strictness
    2. Added an env var to the workflow to easily control the similarity
       ratio

    Changes required to incorporate 1)
    ---------------------------------
    3. Added string-similarity-js package in package.json

Fixes hyperledger-cacti#3470

Signed-off-by: jagpreetsinghsasan <[email protected]>
jagpreetsinghsasan pushed a commit to jagpreetsinghsasan/cactus that referenced this issue Sep 2, 2024
    Primary Changes
    ---------------
    1. Added a commit-pr similarity ratio to lessen the check strictness
    2. Added an env var to the workflow to easily control the similarity
       ratio

    Changes required to incorporate 1)
    ---------------------------------
    3. Added string-similarity-js package in package.json

Fixes hyperledger-cacti#3470

Signed-off-by: jagpreetsinghsasan <[email protected]>
petermetz pushed a commit to jagpreetsinghsasan/cactus that referenced this issue Sep 4, 2024
    Primary Changes
    ---------------
    1. Added a commit-pr similarity ratio to lessen the check strictness
    2. Added an env var to the workflow to easily control the similarity
       ratio

    Changes required to incorporate 1)
    ---------------------------------
    3. Added string-similarity-js package in package.json

Fixes hyperledger-cacti#3470

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 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Hacktoberfest Hacktoberfest participants are welcome to take a stab at issues marked with this label. P3 Priority 3: Medium Tests Anything related to tests be that automatic or manual, integration or unit, etc.
Projects
None yet
2 participants