You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Is your feature request related to a problem? Please describe.
I've mentioned this in passing in #79234 but I think it's worth opening an issue for this for some extra visibiltiy.
I'll copy paste thence:
skip_duplicate check actually doesn't do what we want it to do. For example it thinks that CMakeLists.txt changed in this JSON-only PR: https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/12826245046?pr=79204
This is because it checks the difference between the branch base and the merge pr, which includes all the other changes between when the branch was created and now.
skip_duplicates also can fail the other direction too. For example the intent is there for draft prs to not trigger clang-tidy checks. However that means that one can create a draft PR, skip clang-tidy checks, and then un-draft it, and skip the checks again because skip_duplicates treats it as a duplicate (see this check on this PR itself https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/12848556910?pr=79234)
Now, we are also not abiding to what skip_duplicates tells us to do, because (i think) it sets the should_skip to "Yes" and we later check whether should_skip == "true" and assuming we should never skip things... (see last link above again)
This is all rather broken overall...
Solution you would like.
Re-evalute the usage of skip_duplicates in our workflow, and realign the code with our goals.
The basic approach is obvious (test what needs testing, don't test what doesn't need testing, duh), but the devil is in the details (and it looks pretty thorny down there)
(Separately, I'm not even sure while I'm making this issue. I guess I just got inspired by the "Notify PR authors of merge issues." one. Feel free to close and stuff)
The text was updated successfully, but these errors were encountered:
Is your feature request related to a problem? Please describe.
I've mentioned this in passing in #79234 but I think it's worth opening an issue for this for some extra visibiltiy.
I'll copy paste thence:
skip_duplicate check actually doesn't do what we want it to do. For example it thinks that CMakeLists.txt changed in this JSON-only PR: https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/12826245046?pr=79204
This is because it checks the difference between the branch base and the merge pr, which includes all the other changes between when the branch was created and now.
skip_duplicates also can fail the other direction too. For example the intent is there for draft prs to not trigger clang-tidy checks. However that means that one can create a draft PR, skip clang-tidy checks, and then un-draft it, and skip the checks again because
skip_duplicates
treats it as a duplicate (see this check on this PR itself https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/12848556910?pr=79234)Now, we are also not abiding to what
skip_duplicates
tells us to do, because (i think) it sets theshould_skip
to"Yes"
and we later check whethershould_skip == "true"
and assuming we should never skip things... (see last link above again)This is all rather broken overall...
Solution you would like.
Re-evalute the usage of skip_duplicates in our workflow, and realign the code with our goals.
The basic approach is obvious (test what needs testing, don't test what doesn't need testing, duh), but the devil is in the details (and it looks pretty thorny down there)
Describe alternatives you have considered.
No response
Additional context
part of the reason we use skip_duplicate is to handle "required" checks that we also want to skip conditionally - see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks
(Separately, I'm not even sure while I'm making this issue. I guess I just got inspired by the "Notify PR authors of merge issues." one. Feel free to close and stuff)
The text was updated successfully, but these errors were encountered: