ci(shellcheck): first version #1056
Conversation
|
Getting some ShellCheck CI will be an awesome addition. Thank you! It seems ShellCheck v0.10.0 is out, so assuming this action will use an up-to-date version, I think this is mostly good! Before merging, there are some steps need to take:
I am wondering, though:
Please correct me if I am wrong. ShellCheck takes a couple minutes on my machine to check SteamTinkerLaunch, this could exhaust all minutes easily if executed on PR per-commit, right? Is there anything we could change here? I guess we can always change this later if it becomes a problem, and I don't have any billing information attached to GitHub so if the monthly limit runs out I guess ShellCheck just won't run on CI. But I just thought I'd mention it and see if you had any comments to make :-) Running it on master each merge, though, is a solid plan. 👍 I have no experience with CI, but the yaml looks fine. Could you point me to some docs/info on actions, generally? I'd like a bit of background reading for my own benefit :-) |
|
ShellCheck v0.10.0 is significantly faster for me, taking on average around 35 seconds with far more consistency in that time (usually only varying by a few seconds). ShellCheck v0.8.0 takes around 70 seconds, sometimes up to 120 seconds! Of course CI will likely be slower than my machine, but this should mean v0.10.0 is faster. I guess we'll just have to see how long the ShellCheck action. GitHub offers 2,000 free minutes per-account per-month, and the only two things I'll have with CI are SteamTinkerLaunch and DumpSteamCollections, the latter will not be built very often. If it takes 2 minutes for ShellCheck, that gives us 1,000 runs. If there are many commits with CI running against each one, that'll add up... Maybe just having it on merges to master is an acceptable compromise here? |
|
#1064 fixes the ShellCheck warnings that weren't present in v0.8.0, so once that's merged and we get a bit more discussion on when to run the Action, we can have ShellCheck CI! 🥳 If you can't tell, I'm pretty excited to get this in 😅 |
|
A solution for time, is to move this repo to an organization (also with dumpsteamcollections and the other related tool) so they are on another user. In case in the CI it is possible to set the shellcheck version https://github.com/ludeeus/action-shellcheck/tree/master?tab=readme-ov-file#run-a-specific-version-of-shellcheck |
I've thought about this, but I'd prefer not. This is just a hobby project after all :-)
Yes, I'm more worried about commits. That is, there could conceivably be over 2,000 commits to PRs per month. We also don't know how long it will take on CI, but I imagine CI is slower than my PC with a Ryzen 3700X, and here it takes over a minute. If it takes two minutes on CI, we're down to 1,000 commits per month. If every PR had 4 commits + the merge commit CI, that's 200 PRs per month. And all that also doesn't include the other projects, of course. I don't know if it will be a problem but it is something I want to be mindful of is all 🙂 CI is great but we'll have a limit, and I'm just wondering if perhaps only running on master is suitable (users should be using ShellCheck before merging anyway, and I usually clone and run it against PRs too, and would continue to do so even with CI). |
|
I never saw issues on open source project and in case you reach the limit for that month you don't have more actions. A solution I use to is usually do PRs (not commits on master directly), and when is time to merge them just a squash merge so there are less commits and the history is just clear. Anyway you can remove the pull request supports when you want if you see that is too much. |
Yeah, I think this is a good solution. Basically wait and see if it causes problems. To confirm, if I find that we end up using too much on PRs, I would remove this and set the Action to only run on merges to master by removing the following lines from the yml: pull_request:
types: [opened, reopened] |
|
Looking at the Readme for the project, it appears it will use the latest stable ShellCheck. However we should probably pin to ShellCheck v0.10.0. I will push a change to add the |
|
Think the YAML is valid. Followed the instructions on the ludeeus/action-shellcheck Readme. Will do a quick pass and then merge! |
|
The whole ShellCheck took ~50s on CI for one PR: https://github.com/sonic2kk/steamtinkerlaunch/actions/runs/8334176247 Which in my opinion is pretty good. Something I entirely overlooked is that the CI will only run when the PR is first opened, not on push, which I assumed it would. I think that's where our confusion came from earlier, I totally misunderstood. It looks like you can also run CI on push, but that might be a bit much. You can, however, run CI on a lot of actions. I wonder if we could run CI on a certain label being attached, such as "Run ShellCheck". You can run CI on a label being attached but I'm not sure if you can check which one specifically. I think this would be useful. Instead of running on PR open/reopen, or on each push, only running CI on a specific label would be awesome. That way I can label PRs and get ShellCheck to run. EDIT: This seems possible with: https://stackoverflow.com/a/62331521/1952066 |
|
Just for continuity: I adjusted the workflow defined in this PR to be slightly different. Instead of running on PR open/re-open, we now run only when PRs are labelled with the "shellcheck" label, and on pushes to This is an awesome change, thank you very much! |
|
You are welcome :-) |
|
Huh, it looks like I was wrong, and GitHub Actions are free for public repositories: https://docs.github.com/en/billing/managing-billing-for-github-actions/about-billing-for-github-actions
I found this out when I checked my usage minutes on my account, and found that zero minutes had been used. I guess we don't need to worry about CI on other repos then either. |
|
Better I was memorong something about limits but probably it was on fork, so less stuff to think at the end. |
It is executed only on push on master and on PR, it should help with the development.