Mte90 commented 3 months ago

It is executed only on push on master and on PR, it should help with the development.

sonic2kk commented 3 months ago

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 will need to set the # shellcheck extended-analysis=false at the top of the file, so that ShellCheck can run properly.
I'll need to fix ShellCheck errors that have appeared in 0.10.0. I tested the script quickly with shellcheck --extended-analysis=false steamtinkerlaunch and there are some new warnings popping up that weren't caught before.
I can do both of the above, but can you verify if this action will definitely use ShellCheck v0.10.0? If it isn't, we'll need to wait for a version bump.

I am wondering, though:

It is executed only on push on master and on PR

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. :+1:

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 :-)

sonic2kk commented 3 months ago

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?

sonic2kk commented 3 months ago

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! :partying_face:

If you can't tell, I'm pretty excited to get this in :sweat_smile:

Mte90 commented 3 months ago

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. About the runs, I don't see 2000 commits (or PR) per months in this project together so I don't think that it will be an issue.

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

sonic2kk commented 3 months ago

A solution for time, is to move this repo to an organization

I've thought about this, but I'd prefer not. This is just a hobby project after all :-)

About the runs, I don't see 2000 commits (or PR) per months in this project together

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 :slightly_smiling_face: 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).

Mte90 commented 3 months ago

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. Ideally running also on PR simplify your job on review, so you don't need to check every code.

Anyway you can remove the pull request supports when you want if you see that is too much.

sonic2kk commented 3 months ago

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]

sonic2kk commented 3 months ago

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 with block later tonight, just to be safe in case there are regressions like for v0.9.0 :-)

sonic2kk commented 3 months ago

Think the YAML is valid. Followed the instructions on the ludeeus/action-shellcheck Readme.

Will do a quick pass and then merge!

sonic2kk commented 3 months ago

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

sonic2kk commented 3 months ago

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 master (including PR merges).

This is an awesome change, thank you very much!

Mte90 commented 3 months ago

You are welcome :-)

sonic2kk commented 3 months ago

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

GitHub Actions usage is free for standard GitHub-hosted runners in public repositories

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.

Mte90 commented 3 months ago

Better I was memorong something about limits but probably it was on fork, so less stuff to think at the end.

sonic2kk / steamtinkerlaunch

ci(shellcheck): first version #1056

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! :partying_face: