CI: Add label to scripted-diffs #31089

1. 
   l0rinc commented at 7:57 am on October 15, 2024: contributor

   Add a scripted-diff label for pull requests that contain scripted-diff: commits, to make sure that CI is running them properly. Inspired by #29692

   Note that I’ve used https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/using-github-cli-in-workflows instead of e.g. actions-ecosystem/action-add-labels since it’s a lot simpler, can be tried locally and documents a lot better what we’re doing (it’s also what their example uses in https://docs.github.com/en/actions/use-cases-and-examples/project-management/adding-labels-to-issues).

2. 
   DrahtBot commented at 7:57 am on October 15, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #30997 (build: Switch to Qt 6 by hebasto)
   • #28710 (Remove the legacy wallet and BDB dependency by achow101)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

3. DrahtBot added the label Tests on Oct 15, 2024
4. l0rinc marked this as a draft on Oct 15, 2024
5. l0rinc force-pushed on Oct 15, 2024
6. DrahtBot added the label CI failed on Oct 15, 2024
7. l0rinc force-pushed on Oct 15, 2024
8. l0rinc force-pushed on Oct 15, 2024
9. 
   maflcko commented at 9:15 am on October 15, 2024: member

   Not sure what the goal here would be? Why add a label, when there is already a commit message prefix, which is identical in the meaning?

   It would be better to just fix the bugs like #29692 (comment) to ensure the CI is red when there is an issue.

10. DrahtBot removed the label CI failed on Oct 15, 2024
11. 
    l0rinc commented at 10:07 am on October 15, 2024: contributor

    As an attack vector, people could prefix with scripted–diff (using an en dash or other unicode char which wouldn’t be obvious to a human, but the CI wouldn’t run the provided scripts). Adding a label would provide an extra level of safety to make sure the CI did its job.

    Note that this is still a draft and doesn’t seem to be working yet, comments like these are welcome.

12. l0rinc force-pushed on Oct 15, 2024
13. l0rinc force-pushed on Oct 15, 2024
14. Add label to scripted-diffs

    See: https://docs.github.com/en/actions/use-cases-and-examples/project-management/adding-labels-to-issues

    57e484e290
15. scripted-diff: Fix a few linter errors to dogfood the new gh label

    -BEGIN VERIFY SCRIPT-
    sed -i 's|Unusued|Unused|g' src/cluster_linearize.h
    sed -i 's|neccessary|necessary|g' src/ipc/protocol.h
    sed -i 's|signficant|significant|g' src/pow.cpp
    -END VERIFY SCRIPT-

    582889d008
16. l0rinc force-pushed on Oct 15, 2024
17. 
    maflcko commented at 10:40 am on October 15, 2024: member

    Those checks are part of the bash script itself. If there is a BEGIN, without a matching scripted-diff prefix, or vice-versa, the script should already fail.
18. 
    maflcko commented at 10:41 am on October 15, 2024: member

    If there are (other) bugs, it would be good to enumerate them, with exact steps to reproduce. Then, it would be good to fix them. Once all bugs are fixed, I presume adding a label won’t be needed.
19. 
    l0rinc commented at 10:56 am on October 15, 2024: contributor

    If there is a BEGIN, without a matching scripted-diff

    Which can be replaced with BEGlN (i.e. a lowercase L instead of an I or any other unicode trickery) and reviewers would be fooled into thinking that the scripts were validated by the CI.

20. 
    theuni commented at 2:09 pm on October 15, 2024: member

    Agree with @maflcko that this isn’t adding much as-is. My suggestion was for @DrahtBot to add the label only once it had actually run the scripted-diff successfully.
21. 
    l0rinc commented at 2:58 pm on October 15, 2024: contributor

    My suggestion was for @DrahtBot to add the label only once it had actually run the scripted-diff successfully.

    If you think that’s better than doing it in a separate job, I’ll investigate. @maflcko, would that work with you as well?

22. 
    maflcko commented at 3:02 pm on October 15, 2024: member

    It could still be “hacked” by adding two scripted diffs, one that “fails” and one that passes and adds the label.

    I don’t think this problem can be solved in CI. Only review can catch it.

    Again, my recommendation would be to fix the bug that the exit code is ignored.

23. 
    fanquake commented at 3:04 pm on October 15, 2024: member

    Agree with the other comments. Concept -0
24. 
    l0rinc commented at 3:17 pm on October 15, 2024: contributor

    It could still be “hacked” by adding two scripted diffs, one that “fails” and one that passes and adds the label.

    Valid concern, thanks for the comments, I’ll close this in that case.

25. l0rinc closed this on Oct 15, 2024

    ---

26. l0rinc deleted the branch on Oct 15, 2024

Contributors
l0rinc DrahtBot maflcko theuni fanquake

Labels
Tests