Closed mislav closed 1 month ago
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
I've actually just considered this as well. However, introducing scripts here seems a bit too heavy, also since there are other repositories which could profit from linting the s6 run scripts (e.g. addons-development). Maybe this could be a separate GitHub action at https://github.com/home-assistant/actions.
I've also seen that the current action supports this:
...
with:
additional_files: 'run finish'
that might be a easy start.
Maybe this could be a separate GitHub action at https://github.com/home-assistant/actions.
I'm up for extracting this (or something else that achieves the same) as an action. It's a good idea.
I've also seen that the current action supports this:
I've also seen the additional_files
feature during my dive into ludeeus/action-shellcheck
, but I decided against using it. First of all, it only takes a list of basenames, not full file paths, which I found odd, and matches the names as suffixes to the file name rather than the full name, which is either a bug or a bizarre choice. Second, with this approach we'd have to explicitly allow-list future scripts with unique file names into the linting process, which would be easy to forget. The main point of my PR is that any existing and new scripts in this repository would automatically get linted.
Lastly, the ludeeus/action-shellcheck
actions offers barely any features that are useful to this project, and in my opinion it just places a level of indirection over the linting process that makes it harder to check what's going on. If it offered a way to change its arcane shebang regex as an option rather than list additional basenames to be checked, it would have been more useful, but here I'm proposing to stop using that action in favor of an in-house approach.
but here I'm proposing to stop using that action in favor of an in-house approach.
I must say, ludeeus started hacs, and is quite active in the community, so it semi in-house :smile:
I've just checked, we have at least 4 repos which use ludeeus' shellcheck action right now, we definitely want to share the logic in some way or another. I think I'd prefer to extend his action if it is not too far off from what we need. :thinking:
His shellcheck GH action seems to have quite some traction also outside of the HA community, so your improvements might be beneficial to more people (and vise-versa, other people's improvement might benefit us).
I must say, ludeeus started hacs, and is quite active in the community, so it semi in-house 😄
Ah okay, that is useful context! Thanks for the feedback. I am going to open an alternative PR to this one but that sticks with the same action.
Issue https://github.com/home-assistant/addons/issues/3801 describes a regression where a syntax error was introduced into a bash script that led to an addon startup issue. Because this repository has a linting step, I've looked into how it was that this syntax error wasn't caught by shellcheck. It turned out that a lot of script files in this repository—78 files, to be exact—were not linted due to Home Assistant's unusual shebang:
The "lint" workflow that runs for all pull requests in this repository used the
ludeeus/action-shellcheck@2.0.0
action as a shellcheck wrapper. However, that wrapper only lints files with known extensions and known shebangs, leading to lots of of scripts in this repository not being subject to linting even though they are technically bash scripts.Furthermore, there was no visibility into this linting problem, because the linting step neither printed the list of files processed nor was there a way to run the linter script locally.
This change:
Replaces the usage of the
ludeeus/action-shellcheck@2.0.0
action with an inline script that is runnable locally on both Linux and macOS;Ensures that all script files in the repository are subject to linting regardless of their shebang;
Uses
shellcheck
that comes preinstalled in GitHub Actions runners rather than downloading it every time;Reformats shellcheck output with Actions "workflow commands" syntax, which should lead to nicer formatting of linting errors in GitHub pull requests view.
Here is the diff of the list of all filenames linted before vs after this change
```diff --- scan-before.txt 2024-10-17 16:53:17 +++ scan-after.txt 2024-10-17 16:52:15 @@ -1,0 +2,3 @@ +./assist_microphone/rootfs/etc/s6-overlay/s6-rc.d/assist_microphone/finish +./assist_microphone/rootfs/etc/s6-overlay/s6-rc.d/assist_microphone/run +./assist_microphone/rootfs/etc/s6-overlay/s6-rc.d/discovery/run @@ -2,0 +6,3 @@ +./cec_scan/rootfs/etc/services.d/cec-scan/run +./configurator/rootfs/etc/s6-overlay/s6-rc.d/configurator/finish +./configurator/rootfs/etc/s6-overlay/s6-rc.d/configurator/run @@ -5,0 +12 @@ +./deconz/rootfs/etc/services.d/deconz/discovery @@ -6,0 +14 @@ +./deconz/rootfs/etc/services.d/deconz/run @@ -7,0 +16,4 @@ +./deconz/rootfs/etc/services.d/nginx/run +./deconz/rootfs/etc/services.d/otau-ikea/run +./deconz/rootfs/etc/services.d/otau-ledvance/run +./deconz/rootfs/etc/services.d/websockify/run @@ -10,0 +23,3 @@ +./dnsmasq/rootfs/etc/services.d/dnsmasq/run +./duckdns/rootfs/etc/s6-overlay/s6-rc.d/duckdns/finish +./duckdns/rootfs/etc/s6-overlay/s6-rc.d/duckdns/run @@ -13,0 +29 @@ +./google_assistant/rootfs/etc/services.d/google-assistant/run @@ -15,0 +32,12 @@ +./letsencrypt/rootfs/etc/services.d/lets-encrypt/run +./mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-core/finish +./mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-core/run +./mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-lock-core/finish +./mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-lock-core/run +./mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-lock-post/finish +./mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-lock-post/run +./mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-post/finish +./mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-post/run +./mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-pre/run +./mariadb/rootfs/usr/bin/lock-tables-for-backup +./mariadb/rootfs/usr/bin/unlock-tables-for-backup @@ -16,0 +45 @@ +./matter_server/rootfs/etc/s6-overlay/s6-rc.d/matter-server/run @@ -17,0 +47 @@ +./matter_server/rootfs/etc/s6-overlay/scripts/matter-server-discovery @@ -20,0 +51 @@ +./mosquitto/rootfs/etc/services.d/mosquitto/discovery @@ -21,0 +53 @@ +./mosquitto/rootfs/etc/services.d/mosquitto/run @@ -22,0 +55,4 @@ +./mosquitto/rootfs/etc/services.d/nginx/run +./nginx_proxy/rootfs/etc/periodic/daily/check_certificate_renewal +./openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/mdns/finish +./openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/mdns/run @@ -23,0 +60,4 @@ +./openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/finish +./openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/run +./openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-web/finish +./openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-web/run @@ -24,0 +65 @@ +./openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/socat-otbr-tcp/run @@ -28,0 +70,7 @@ +./openthread_border_router/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up +./openwakeword/rootfs/etc/s6-overlay/s6-rc.d/discovery/run +./openwakeword/rootfs/etc/s6-overlay/s6-rc.d/openwakeword/finish +./openwakeword/rootfs/etc/s6-overlay/s6-rc.d/openwakeword/run +./piper/rootfs/etc/s6-overlay/s6-rc.d/discovery/run +./piper/rootfs/etc/s6-overlay/s6-rc.d/piper/finish +./piper/rootfs/etc/s6-overlay/s6-rc.d/piper/run @@ -29,0 +78,5 @@ +./samba/rootfs/etc/s6-overlay/s6-rc.d/init-smbd/run +./samba/rootfs/etc/s6-overlay/s6-rc.d/nmbd/finish +./samba/rootfs/etc/s6-overlay/s6-rc.d/nmbd/run +./samba/rootfs/etc/s6-overlay/s6-rc.d/smbd/finish +./samba/rootfs/etc/s6-overlay/s6-rc.d/smbd/run @@ -31,0 +85,4 @@ +./silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/cpcd/finish +./silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/cpcd/run +./silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/mdns/finish +./silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/mdns/run @@ -32,0 +90,4 @@ +./silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/finish +./silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/run +./silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/otbr-web/finish +./silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/otbr-web/run @@ -33,0 +95,3 @@ +./silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/socat-cpcd-tcp/run +./silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/zigbeed/finish +./silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/zigbeed/run @@ -34,0 +99 @@ +./silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/cpcd-config-up @@ -38,0 +104 @@ +./silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up @@ -39,0 +106 @@ +./silabs_flasher/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up @@ -44,0 +112 @@ +./ssh/rootfs/etc/services.d/sshd/run @@ -45,0 +114 @@ +./ssh/rootfs/etc/services.d/ttyd/run @@ -46,0 +116,2 @@ +./ssh/rootfs/usr/local/bin/reboot +./ssh/rootfs/usr/local/bin/shutdown @@ -49,0 +121,8 @@ +./vlc/rootfs/etc/s6-overlay/s6-rc.d/nginx/finish +./vlc/rootfs/etc/s6-overlay/s6-rc.d/nginx/run +./vlc/rootfs/etc/s6-overlay/s6-rc.d/vlc/finish +./vlc/rootfs/etc/s6-overlay/s6-rc.d/vlc/run +./vlc/rootfs/etc/s6-overlay/scripts/vlc-discovery +./whisper/rootfs/etc/s6-overlay/s6-rc.d/discovery/run +./whisper/rootfs/etc/s6-overlay/s6-rc.d/whisper/finish +./whisper/rootfs/etc/s6-overlay/s6-rc.d/whisper/run @@ -51,0 +131 @@ +./zwave_js/rootfs/etc/services.d/zwave_js/discovery @@ -52,0 +133 @@ +./zwave_js/rootfs/etc/services.d/zwave_js/run ```TODO:
Summary by CodeRabbit
New Features
lint.sh
) for linting shell scripts in the repository.Bug Fixes
Documentation