Skip to content

Some shellcheck fixes#6246

Merged
orbitcowboy merged 2 commits intomainfrom
shellcheck_fixes
Apr 6, 2024
Merged

Some shellcheck fixes#6246
orbitcowboy merged 2 commits intomainfrom
shellcheck_fixes

Conversation

@orbitcowboy
Copy link
Copy Markdown
Collaborator

No description provided.

@orbitcowboy orbitcowboy merged commit 8f4a598 into main Apr 6, 2024
@orbitcowboy orbitcowboy deleted the shellcheck_fixes branch April 6, 2024 13:16
@firewave
Copy link
Copy Markdown
Collaborator

firewave commented Apr 6, 2024

We have shellcheck in the CI but currently have several checks disabled. If you think there is something worth enabling please do so that it is actually enforced. These are mainly disabled because the step existed since quite a while but did not fail when it raised a warning so most were disabled instead of fixing them.

@orbitcowboy
Copy link
Copy Markdown
Collaborator Author

We have shellcheck in the CI but currently have several checks disabled. If you think there is something worth enabling please do so that it is actually enforced. These are mainly disabled because the step existed since quite a while but did not fail when it raised a warning so most were disabled instead of fixing them.

Yes, I saw that we have mostly deactivated it. There are a lot of portability warnings from shellcheck that can be fixed, especially in this script. On the long run, I think it's a good attitude to become (mostly) shellcheck clean in our scripts as well.

@firewave
Copy link
Copy Markdown
Collaborator

firewave commented Apr 7, 2024

Yes, I saw that we have mostly deactivated it. There are a lot of portability warnings from shellcheck that can be fixed, especially in this script. On the long run, I think it's a good attitude to become (mostly) shellcheck clean in our scripts as well.

Enabling them just one at a time also gets us there at the end.

Just be sure to test it. I (barely) remember that it had some false positives which actually broke scripts. That has been ages ago so those might have been fixed since.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants