Skip to content

Sync the pre-commit checks with what CI is doing#267

Merged
loverdos merged 47 commits intomainfrom
feat/pre-commit-sync-CI
Aug 11, 2025
Merged

Sync the pre-commit checks with what CI is doing#267
loverdos merged 47 commits intomainfrom
feat/pre-commit-sync-CI

Conversation

@loverdos
Copy link
Copy Markdown
Contributor

@loverdos loverdos commented Aug 1, 2025

Description and remarks

  • Run the pre-commit hook in CI. It basically supersedes most other tests, hence some deletions of .github/workflows/sdk.yml etc.
  • Run times are from 6-8 minutes to 24-26 minutes depending on a) if the cache kicks in, b) if GitHub is having a good day and does not fail cache saving.
  • When the cache works, building and testing everything is faster than selectively building/testing individual crates, not to mention that we always get the dependencies right.
  • Add one pre-commit action to handle a CLI integration test (for CLI completions in bash)
  • Add one pre-commit check for executable scripts. We are now being strict, in the sense if something looks like a shell script, it must be executable.
  • Introduce a two-layered CI cache for a) the main branch, b) PRs. Both interoperate.
  • Unfortunately, the caching system of GitHub is unreliable. Sometimes caches are saved, sometimes they are not. Still, anecdotally we are doing better than the current (previous to this PR) setup, where we observe that generally cache does not work the way we use it.
  • I have come to the conclusion that it's better to use actions/cache manually, rather than relying on marketplace GitHub actions, such as actions-rust-lang/setup-rust-toolchain or Swatinem/rust-cache. Generally, GitHub actions are not composable.
  • At some point, when everything seemed stable, I introduced sccache. It looked promising, until I started getting weird errors out of the blue, like warning: ring@0.17.14: sccache: error: failed to create directory ~/.cache/sccache/preprocessor, and so I abandoned sccache altogether. Life is short.
  • In general, though this was an interesting learning exercise, it has mostly been full of frustrations. I am certain I did not get everything right and there is room for improvement, but I prefer we move to another system/way of doing CI than spend time fighting GitHub actions. Honestly, good old Jenkins on a vanilla VM would be better/more reliable (but I am not saying we should do that).
  • I tried several variations of making reusable GitHub actions, but I decided to stop here. Please do not recommend more in your review. Feel free to open a ticket to address this in the future.
  • There are some other workflows that overlap, e.g. checking for typos. TODO Improve quality without remorse (nexus-sdkv0.3.0) #272
  • All of these notes will go to the commit message.

References

Checklist

  • keep a changelog
  • write tests
  • create tickets for TODO: <> statements
  • create or update documentation

@loverdos loverdos added this to the v0.2.0 milestone Aug 1, 2025
@loverdos loverdos linked an issue Aug 1, 2025 that may be closed by this pull request
@loverdos loverdos self-assigned this Aug 1, 2025
Comment thread .pre-commit/pre-commit
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@loverdos loverdos changed the title WIP Sync the pre-commit checks with what CI is doing Aug 8, 2025
@loverdos loverdos marked this pull request as ready for review August 8, 2025 08:46
@loverdos loverdos requested a review from a team August 8, 2025 08:46
Comment thread helpers/helpers.just
@loverdos loverdos removed this from the v0.2.0 milestone Aug 8, 2025
Copy link
Copy Markdown
Contributor

@zanicar zanicar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Although someone with more CI experience may want to confirm the details.

@loverdos loverdos requested a review from a team August 11, 2025 14:09
Copy link
Copy Markdown
Contributor

@zanicar zanicar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@loverdos loverdos merged commit f131bd2 into main Aug 11, 2025
13 checks passed
@loverdos loverdos deleted the feat/pre-commit-sync-CI branch August 11, 2025 15:13
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.

Sync the pre-commit checks with what CI is doing.

4 participants