-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-141004: GHA: Run check-c-api-docs check on docs-only PRs
#143573
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
base: main
Are you sure you want to change the base?
Conversation
ZeroIntensity
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me, but I'd like one of the GHA experts to have a look.
|
@webknjaz, does this look interesting? |
|
Personally, I've been leaning towards having a single workflow tool target invocation per job. In this case it's one make command (I've implemented my So yes, it's a good idea — it can run a check that would otherwise be skipped, sometimes. But I also foresee better responsiveness even in full runs. This could be taken farther at some point (follow-ups) but it's well-scoped as it is. One thing I'd ask, though would be moving the new job into a
In that case, I'd maybe reduce the job timeout to 5 minutes or less. This tends to improve responsiveness too, plus catch problems with sudden slowdowns w/o wasting too much CPU. |
Done :-) |
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
webknjaz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ready, I left a style note, though. But that's inconsistent across the CI definitions anyway, so not important.
| needs.build-context.outputs.run-tests == 'true' | ||
| || needs.build-context.outputs.run-docs == 'true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(personal style preference, optional)
| needs.build-context.outputs.run-tests == 'true' | |
| || needs.build-context.outputs.run-docs == 'true' | |
| fromJSON(needs.build-context.outputs.run-tests) | |
| || fromJSON(needs.build-context.outputs.run-docs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to remain consistent, I'll leave it up to whoever merges, however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this is consistent with a bunch of places while differs from the others. I think GHA infra could use some linting and will probably look into that later. But again, this is probably something that would be improved in follow-ups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a truthiness check, thing == 'true' is clearer to me than fromJSON(thing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Hugo that is indeed more readable/logical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright.. String comparison it is, then. It's just that the readability is poor when multiple conditionals are chained and having function calls makes it a little bit better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright.. String comparison it is, then. It's just that the readability is poor when multiple conditionals are chained and having function calls makes it a little bit better.
As mentioned in #143564 (review) by @ZeroIntensity:
We could keep it in the
check-generated-filesjob, but that would make it quite messy as all the other checks would have to be excluded in the case when onlyrun-docsis true. And, to avoid having to configure, I run the script directly (since it usesPYTHON_FOR_REGENanyway, there shouldn't be a difference).On this PR, the new job ran in 13 seconds.