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 to ignore dependabot pull requests #3469

Closed
petermetz opened this issue Aug 8, 2024 · 3 comments · Fixed by #3521
Closed

ci(github): commit parity check to ignore dependabot pull requests #3469

petermetz opened this issue Aug 8, 2024 · 3 comments · Fixed by #3521
Assignees
Labels
Hacktoberfest Hacktoberfest participants are welcome to take a stab at issues marked with this label. P4 Priority 4: Low Tests Anything related to tests be that automatic or manual, integration or unit, etc.
Milestone

Comments

@petermetz
Copy link
Member

Description

Dependabot pull requests are failing the PR commit parity check and the only way to fix them is by manually editing every pull request the bots make which is a drain on our resources and I'm not sure it's worth it for this particular category of pull requests. Here's why:

The dependabot pull requests are always the same in the sense that they are just bumping dependences so their complexity remains very low, forever.

With this in mind the easiest shortcut for us right now is to just add a configuration option to the commit parity check which allows us to tell it to ignore pull requests from certain authors (and for now the one item we'll put on that list is dependabot).

Acceptance Criteria

  1. It is possible to extend the list of bot authors later without having to change source code of the check itself (env variables are a great way to introduce this configurability through a CVS such as CACTI_CUSTOM_CHECK_PR_COMMIT_PARITY_EXEMPTED_AUTHORS=dependabot,some-other-bot-1,yet_another_bot_2,etc.
  2. The exempted authors get passed for the commit parity check.
  3. The check is still very much in effect for everyone else (humans) who are making more complex contributions which require that we make sure the commit message contains the full context that the PR description had in it.
@petermetz petermetz added 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. P4 Priority 4: Low labels Aug 8, 2024
@petermetz petermetz added this to the v2.0.0 milestone Aug 8, 2024
@jagpreetsinghsasan jagpreetsinghsasan self-assigned this Sep 2, 2024
@jagpreetsinghsasan
Copy link
Contributor

Hi @petermetz
If we have a workflow level conditional statement to see if the PR creator is a bot or not, then it would be the best as we can see the skipped status of the workflow
However, there is a limitation of github workflow, that we cannot use env vars in conditional statements:
image

Setting up as a part of github context is resource intensive (as we need to run a job to set it up)
And if we setup it as a part of vars context, then that would have to maintained as a part of repository variables under Actions and Secrets section of the repo settings.

Can we do something like this (less elegant, but least resource intensive way imo)?

image

(My main focus point here is that having a github workflow level conditional statement would be the most informative and least resource extensive way of doing this)

@petermetz
Copy link
Member Author

@jagpreetsinghsasan Agreed on all counts. I think the hardcoded bot names are a good trade-off for now we only have dependabot on that list of bots anyway (that I know of).

I also agree very much that the resource usage of it is already too high, ideally a baseline check like this wouldn't even need to run yarn install nor yarn configure and be powered by a much smaller footprint. I know that we need the install because of that string similarity library I suggested to use but maybe we can have the robots write a quick and naive implementation of string similarity that is good enough, something like this:

https://chatgpt.com/share/40999856-36eb-4f41-a841-2e0d6af37eca

If we could make it so that the commit parity check is just a git clone ... && node ./tools/check-commit-parity.js then it would save a lot on resources and also provide rapid feedback to people while their mind is fresh on the task of having opened the PR (if we need 5-10 minutes to tell them that the check has failed they probably already closed the browser tab and moved on to some other task)

@jagpreetsinghsasan
Copy link
Contributor

@jagpreetsinghsasan Agreed on all counts. I think the hardcoded bot names are a good trade-off for now we only have dependabot on that list of bots anyway (that I know of).

I also agree very much that the resource usage of it is already too high, ideally a baseline check like this wouldn't even need to run yarn install nor yarn configure and be powered by a much smaller footprint. I know that we need the install because of that string similarity library I suggested to use but maybe we can have the robots write a quick and naive implementation of string similarity that is good enough, something like this:

https://chatgpt.com/share/40999856-36eb-4f41-a841-2e0d6af37eca

If we could make it so that the commit parity check is just a git clone ... && node ./tools/check-commit-parity.js then it would save a lot on resources and also provide rapid feedback to people while their mind is fresh on the task of having opened the PR (if we need 5-10 minutes to tell them that the check has failed they probably already closed the browser tab and moved on to some other task)

@petermetz I think the library we used also uses the same algorithm in the backend. True that the dependencies should not exist otherwise devs will just leave it as it is and move on the other tasks. I will implement the same (I haven't tried using LLMs outside generating images and posters, this is the first time I am seeing it code XD)

jagpreetsinghsasan pushed a commit to jagpreetsinghsasan/cactus that referenced this issue Sep 5, 2024
    Primary Changes
    ---------------
    1. Updated the workflow to include a skip
       when the PR author is dependabot
    2. Updated the pr-commit parity script to
       include Levenshtein Distance string metric
       instead of importing a package to reduce
       workflow runtime

    Changes required to incorporate 1)
    3. Updated workflows/pr-commit-parity.yaml with
       a conditional statement

    Changes required to incorporate 2)
    4. Updated the script with the functions,
       levensheteinDistance and stringSimilarity
       to have the required functionality
    5. Updated the package.json with removal of the
       package dependency of string-similarity-js
    6. Updated the workflow and removed steps to
       parse the project, thus reducing workflow
       runtime

Fixes hyperledger#3469

Signed-off-by: jagpreetsinghsasan <jagpreetsinghsasan@accenture.com>
jagpreetsinghsasan pushed a commit to jagpreetsinghsasan/cactus that referenced this issue Sep 5, 2024
    Primary Changes
    ---------------
    1. Updated the workflow to include a skip
       when the PR author is dependabot
    2. Updated the pr-commit parity script to
       include Levenshtein Distance string metric
       instead of importing a package to reduce
       workflow runtime

    Changes required to incorporate 1)
    3. Updated workflows/pr-commit-parity.yaml with
       a conditional statement

    Changes required to incorporate 2)
    4. Updated the script with the functions,
       levensheteinDistance and stringSimilarity
       to have the required functionality
    5. Updated the package.json with removal of the
       package dependency of string-similarity-js
    6. Updated the workflow and removed steps to
       parse the project, thus reducing workflow
       runtime

Fixes hyperledger#3469

Signed-off-by: jagpreetsinghsasan <jagpreetsinghsasan@accenture.com>
jagpreetsinghsasan pushed a commit to jagpreetsinghsasan/cactus that referenced this issue Sep 5, 2024
    Primary Changes
    ---------------
    1. Updated the workflow to include a skip
       when the PR author is dependabot
    2. Updated the pr-commit parity script to
       include Levenshtein Distance string metric
       instead of importing a package to reduce
       workflow runtime

    Changes required to incorporate 1)
    3. Updated workflows/pr-commit-parity.yaml with
       a conditional statement

    Changes required to incorporate 2)
    4. Updated the script with the functions,
       levensheteinDistance and stringSimilarity
       to have the required functionality
    5. Updated the package.json with removal of the
       package dependency of string-similarity-js
    6. Updated the workflow and removed steps to
       parse the project, thus reducing workflow
       runtime

Fixes hyperledger#3469

Signed-off-by: jagpreetsinghsasan <jagpreetsinghsasan@accenture.com>
petermetz pushed a commit to jagpreetsinghsasan/cactus that referenced this issue Sep 9, 2024
    Primary Changes
    ---------------
    1. Updated the workflow to include a skip
       when the PR author is dependabot
    2. Updated the pr-commit parity script to
       include Levenshtein Distance string metric
       instead of importing a package to reduce
       workflow runtime

    Changes required to incorporate 1)
    3. Updated workflows/pr-commit-parity.yaml with
       a conditional statement

    Changes required to incorporate 2)
    4. Updated the script with the functions,
       levensheteinDistance and stringSimilarity
       to have the required functionality
    5. Updated the package.json with removal of the
       package dependency of string-similarity-js
    6. Updated the workflow and removed steps to
       parse the project, thus reducing workflow
       runtime

Fixes hyperledger#3469

Signed-off-by: jagpreetsinghsasan <jagpreetsinghsasan@accenture.com>
petermetz pushed a commit that referenced this issue Sep 9, 2024
    Primary Changes
    ---------------
    1. Updated the workflow to include a skip
       when the PR author is dependabot
    2. Updated the pr-commit parity script to
       include Levenshtein Distance string metric
       instead of importing a package to reduce
       workflow runtime

    Changes required to incorporate 1)
    3. Updated workflows/pr-commit-parity.yaml with
       a conditional statement

    Changes required to incorporate 2)
    4. Updated the script with the functions,
       levensheteinDistance and stringSimilarity
       to have the required functionality
    5. Updated the package.json with removal of the
       package dependency of string-similarity-js
    6. Updated the workflow and removed steps to
       parse the project, thus reducing workflow
       runtime

Fixes #3469

Signed-off-by: jagpreetsinghsasan <jagpreetsinghsasan@accenture.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hacktoberfest Hacktoberfest participants are welcome to take a stab at issues marked with this label. P4 Priority 4: Low Tests Anything related to tests be that automatic or manual, integration or unit, etc.
Projects
2 participants