-
Notifications
You must be signed in to change notification settings - Fork 176
chore(repo): add codespell config and harden CI workflows #572
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
Author: rezky_nightky <with dot rezky at gmail dot com> Repository: dkms Branch: pr/ci-tooling Signing: GPG (4B65AAC2) HashAlgo: BLAKE3 [ Block Metadata ] BlockHash: d80fe14783de0990ee63a0696869d52cbb443d6f0f1d658be4a6211afe23f0da PrevHash: 9959f0b6d0f18f79266239652f31ec7090f8ed0a71d470edb4f4b87742266465 PatchHash: 28c1869c970b2617f550bfb5135f5b1fd3d0288840e80c75167f7a33a3dbe8ec FilesChanged: 3 Lines: +244 / -115 Timestamp: 2026-01-09T13:05:37Z Signature1: 97bf83a7b2e9f96a1b5ab9f2d4e37334147c2c52ed0e39f686585e4b1f319c8b Signature2: 5bc954b94690fa04b00a5b7d3c7e57ccd2783e5db10f6102753b70472173236d
scaronni
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.
To be honest I find this pull request more detrimental than anything, it just makes things less readable than before and just changes echo with printf and redirects some command expansion.
What exactly are you trying to achieve?
.github/workflows/shellcheck.yml
Outdated
| name: Shellcheck | ||
|
|
||
| on: | ||
| 'on': |
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.
What's the reason for these quotes?
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.
That was done to satisfy YAML linters that treat on as a reserved boolean-ish key. Since it hurts readability here, I reverted it.
.github/workflows/tests.yml
Outdated
| - {name: "ubuntu", tag: "22.04"} | ||
| - name: almalinux | ||
| tag: "10" | ||
| image: "almalinux:10" |
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 is actually less readable than before, and having also an image is completely redundant. It's just name+tag in most cases.
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.
Agreed; reverted to the original matrix format.
.github/workflows/tests.yml
Outdated
| runs-on: ubuntu-24.04 | ||
| container: | ||
| image: ${{ matrix.distro.url }}${{ matrix.distro.name }}:${{ matrix.distro.tag }} | ||
| image: ${{ matrix.distro.image }} |
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.
Same comment as above.
.github/workflows/tests.yml
Outdated
| make install | ||
| - name: Install openSUSE leap dependencies | ||
| if: contains(matrix.distro.name, 'opensuse') |
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.
And now, since you've added the URL to the image tag, you have to switch to contains just for the case of images with an URL in it.
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.
Agreed; reverted the URL/image refactor, so the original condition is back.
| @@ -0,0 +1,2 @@ | |||
| [codespell] | |||
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 don't see codespell being enabled anywhere.
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.
Correct — this PR only adds the config file. If you want, I can add a minimal codespell CI workflow; otherwise I can remove .codespellrc.
.github/workflows/tests.yml
Outdated
| - uses: actions/checkout@v6 | ||
|
|
||
| - name: Install dependencies for Red Hat based distributions | ||
| if: >- |
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.
What's the point of this and all other shortening of the lines? It's just less readable and I'm pretty sure nobody works anymore on a 80x24 terminal.
.github/workflows/tests.yml
Outdated
| # There should be two entries - "modules" and the kernel we installed | ||
| if [ $(echo "${kernels}" | wc -l) -ne 2 ]; then | ||
| # There should be two entries - "modules" and the kernel we installed | ||
| if [ "$(printf '%s\n' "${kernels}" | wc -l)" -ne 2 ]; then |
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.
Am I right or these printf instead of an echo is the only real change in this pull request?
Author: rezky_nightky <with dot rezky at gmail dot com> Repository: dkms Branch: pr/ci-tooling Signing: GPG (4B65AAC2) HashAlgo: BLAKE3 [ Block Metadata ] BlockHash: 720b7d737e205ae56df9366ba97c7208cd46a9e9d9b18d4d6f245ca59b23e775 PrevHash: a1afbf1c5f14c341956276b162e43ac622a813cfbdd0e3090e4a56ecb99e007f PatchHash: 524e0098616e26f585f9ea05b1bb45625246e7d3100a9c3b1767befe6f92b3bd FilesChanged: 2 Lines: +115 / -242 Timestamp: 2026-01-09T18:35:03Z Signature1: 44d195d95c491db0a14ab9211500e79d910c1071bd771be53e2c7e5df8b554fe Signature2: 1b1de9d7c55302cb9b52d8fe36e60a745497ccdca1aae14ae8138142974f9634
|
hey check this!. Pushed e4dce5a reverting the workflow refactors back to upstream for readability (removed quoted on, redundant matrix image, line wrapping, contains() workaround, etc.). This also fixes the Gentoo failure: the multi-line gentoo_repo_url='.../'\n 'master.tar.gz' caused master.tar.gz: command not found under sh due to whitespace after the line continuation. Net diff vs main is now only .codespellrc. |
This PR contains CI/tooling-only changes (no runtime behavior changes): .codespellrc plus workflow lint/shell safety cleanups for the GitHub Actions jobs.
Validation: best signal here is CI itself; please see the workflow runs on this PR for confirmation that lint/tests execute cleanly.
Author: rezky_nightky
Repository: dkms
Branch: pr/ci-tooling
Signing: GPG (4B65AAC2)
HashAlgo: BLAKE3
[ Block Metadata ]
BlockHash: d80fe14783de0990ee63a0696869d52cbb443d6f0f1d658be4a6211afe23f0da PrevHash: 9959f0b6d0f18f79266239652f31ec7090f8ed0a71d470edb4f4b87742266465 PatchHash: 28c1869c970b2617f550bfb5135f5b1fd3d0288840e80c75167f7a33a3dbe8ec
FilesChanged: 3
Lines: +244 / -115
Timestamp: 2026-01-09T13:05:37Z
Signature1: 97bf83a7b2e9f96a1b5ab9f2d4e37334147c2c52ed0e39f686585e4b1f319c8b
Signature2: 5bc954b94690fa04b00a5b7d3c7e57ccd2783e5db10f6102753b70472173236d