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

fix comparison used to determine change in invalidations #20

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

KristofferC
Copy link

@KristofferC KristofferC commented Jul 12, 2024

#4 made the comparison go from a number comparison to a string comparison effectively breaking the workflow.

Fixes #16, #8

@KristofferC
Copy link
Author

KristofferC commented Jul 12, 2024

Is it really intended to install this workflow by copy pasting the check into every repo that uses it. A bug in that code (which is the case here) means that every single repo has to update their workflow. Shouldn't this be versioned somehow?

Anyway, there should probably be some mass action that opens PR to fix all the places where this workflow got added.

@KristofferC
Copy link
Author

KristofferC commented Jul 12, 2024

It might be possible to fix the existing actions by being sufficiently clever in updating how these values are stored:

outputs:
total:
description: "Total number of invalidations"
value: ${{ steps.invs.outputs.total }}
deps:
description: "Number of invalidations caused by deps"
value: ${{ steps.invs.outputs.deps }}

so that if a < b then string(a) < string(b) (maybe via zero padding)

@ranocha
Copy link
Contributor

ranocha commented Jul 15, 2024

Would changing

println(io, "total=$(inv_total)")
println(io, "deps=$(inv_deps)")

to something like

           println(io, @sprintf("total=%09d", inv_total)")
           println(io, @sprintf("deps=%09d", inv_deps)")

do this trick?

@KristofferC
Copy link
Author

Maybe, I'm not sure exactly how github treats these things so I think it just has to be tested in practice.

@IanButterworth
Copy link
Member

Has that been tried? It'd be good to get this in given maintenance attention on the action right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] Check does not fail when number of invalidations increases
3 participants