-
Notifications
You must be signed in to change notification settings - Fork 66
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
Change exit code when no namespaces were linted and caching is enabled #453
Conversation
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.
LGTM!
btw, how long is a 'long' CI run?
Over the years I've never used the caching feature since running the test suite generally always is slower than Eastwood, for any non-trivial project.
So, given a CI setup where Eastwood and the test suite are run in parallel, Eastwood is not a blocker and not even the slowest cog in there 😄
Finally, in many monorepo setups, it becomes possible to enable "only run this <test suite / linter / thing>
if this particular sub-project has changed".
So that would allow you to run Eastwood only in the relevant sub-projects for a given changeset. Possibly Polylith has good tooling for that kind of logic.
Cheers - V
src/eastwood/lint.clj
Outdated
@@ -839,7 +840,7 @@ Eastwood has other forms of effective, safe parallelism now. Falling back to seq | |||
(reporting/note reporter "== No namespaces were linted. This might indicate a misconfiguration.")) | |||
{:some-warnings (or (> warning-count 0) | |||
has-errors? | |||
nothing-was-linted?) | |||
(and (zero? modified-since) nothing-was-linted?)) |
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'd suggest to extract the :modified-since 0
to :modified-since default-modified-since
and use (= modified-since default-modified-since)
here
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.
It's funny you should mentioned Polylith, because we're using it. A long linting run in our case is around 3mins for 200 files. We do run Eastwood in parallel with our github matrix projects tests, exactly as you describe it, by determining which projects need building using Polylith, and only building those. And, you're right, Eastwood is not usually the slowest cog either. We was just optimizing our overall build process, the caching is something we thought worth adopting, and was just surprised when the build step failed due to the exit code. Thanks for looking through the PR, and I shall make the change you suggested. Cheers!
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.
3 mins sounds pretty sweet!
The worst times I've witnessed over the years were around the 7-minute mark, maybe 9. Think truly enterprise-sized behemoths.
But if you can optimize things beyond 3m, all the power to you 👍
Yeah, 3 mins is very manageable, but I remember when it was < 1 min in our early days, so it's crept up on us for sure! I made the changes you suggested, and thanks again. |
4fe26dd
to
9740e07
Compare
Thanks to you! I've tagged 1.4.3 - a release should be generated in < 1h. |
No longer consider
No namespaces linted.
as a warning if:only-modified true
and the
.eastwood
file contains a valid timestamp.Linting a large monorepo is time-consuming in CI builds. This can be mitigated by caching the
.eastwood
file between runs on the same branch. However, eastwood returns a non-zero exit code when no namespaces were linted, which is problematic. Rather than ignore the error code and continue the build, it would be better to return a zero error code in this circumstance.Fixes: #454
Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):
Thanks!