build: add make lint-shell and add to PR checks

[jan--f]

This add a shellcheck make target and PR test. Additionall this fixes existing shellcheck issues.

Fixes: #212

Signed-off-by: Jan Fajerski [email protected]

[JoaoBraveCoding]

looks good! just one minor nit

[JoaoBraveCoding]

Not quite sure why we set a version here if we always curl for the latest stable release 🤔 Is the goal that make shellcheck fails when a new version comes out and this way we know we have to update this env var?

[jan--f]

This is indeed a little weird. We use the tool versions for two purposes:

1. Some tools download in specific versions indeed.
2. We use the versions in the github actions to signal when to tool install needs to be redone. The versions are piped into a file and that files hash is used to tag the tools directory. This tools dir is reused until a version changes. That changes the file, which changes the hash, which triggers an action to reinstall the tools.

And yes when a new version is released the install target for shellcheck will fail and let us know to update the version.

[sthaha]

I think we can use the pinned version to construct the url and should consistent results both in CI and locally.

https://github.com/koalaman/shellcheck/releases/download/v$(SHELLCHECK_VERSION)/shellcheck-v$(SHELLCHECK_VERSION).linux.x86_64.tar.xz

[jan--f]

Sure we could but we don't really depend on a specific shellcheck version. The version tracking is really just to support the ghub action tool caching.
This way we'll automatically pull in a new version when its released and we only need to update the version. If we pin it we might end up using an outdated version for a long time when we don't really need to.
The same already happens for other tools (oc, jb and several others)...in some cases even without an version check. I'd rather propose we add the version check to the other tools and only pin to a particular version if required.

[sthaha]

For the record I am not against this patch from merging since it already adds value.

However, I feel we should pin the version for consistency sake and not needing to upgrade the tools at random. I do agree there is a downside to pinning version but the advantage is that we will have consistent behaviour across all environments (dev, ci); i.e. .. if it fails on CI, it would fail on dev as well.

This way we'll automatically pull in a new version when its released and we only need to update the version.

Not really the case, since tools will only be updated if the .github/tools file change

More importantly, unrelated changes shouldn't break CI .. e.g. given the uncertainty that the tool version can change at anytime (whenever tools cache is invalidated), we may have a situation where you update a tool to fix an issue but would also have to fix unrelated shell script.

The same already happens for other tools (oc, jb and several others)...in some cases even without an version check.

I consider this more as an advantage to pinning the version, i.e. we focus on addressing the issues that matter and CI doesn't break because oc or jb has a new release mode.

[jan--f]

I created #241 to follow up on this.

[sthaha]

How about we use the same version as the one declared above to be consistent ?

https://github.com/koalaman/shellcheck/releases/download/$(VERSION)/shellcheck-v$(VERSION).linux.x86_64.tar.xz

[sthaha]

I think this patch breaks hack/kind/setup.sh. Could you please confirm that the script runs fine?

[sthaha]

lgtm