Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: Add a new workflow to check and format shell scripts in util dir #5077

Merged
merged 1 commit into from
Jul 16, 2023

Conversation

starccy
Copy link
Contributor

@starccy starccy commented Jul 13, 2023

Hi,
I've added a new GitHub action workflow to resolve #4752. It utilizes shellcheck and shfmt tools to perform checks and formatting on our shell scripts during a push to the main branch or the creation of a PR.

As it stands, I would appreciate some input on the following aspects that may require further discussion or decision:

  1. Unification of Shell Interpreters: It appears that some of our scripts utilize #!/bin/sh shebang, yet contain certain statements that do not adhere to the POSIX sh standard, as pointed out by shellcheck. I have attached an example of such an occurrence in the CI output from my forked repo here: ShellScript/Check. As a temporary measure, I've configured shellcheck to scrutinize all scripts against the bash rules. Would this be acceptable or should we strictly adhere to the $shebang rules?
  2. Inclusion of Specific shellcheck Rules: Currently, I've left the shellcheck rules to their default settings, which still yield a number of warnings: ShellScript/Check. My plan is to address these in an additional commit, but before proceeding, it would be beneficial to determine which rules we should strictly enforce.
  3. Auto Commit Post shfmt Execution: Once shfmt completes successfully, an automatic commit will be added to the main branch. I was wondering if this practice aligns with our workflow expectations.

echo "## perform a shell format..."
# ignore the error code because `-d` will always return false when the file has difference
find ${{ env.SCRIPT_DIR }} -name "*.sh" -print0 | xargs -0 shfmt -ln=bash -i 4 -ci -w
- name: Commit any changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please replace it by a failure, not an autofix :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I'll remove the autofix code.

@sylvestre
Copy link
Contributor

it didn't run. I guess it is because a new testsuite.
Let's see if it works if I merge it once you fixed the comment

@tertsdiepraam
Copy link
Member

Yes, I think that's due to that security feature of github where it won't tun unmerged workflows.

@starccy
Copy link
Contributor Author

starccy commented Jul 16, 2023

I've removed any write operations. now the job will fail if the code does not meet the formatting specifications.

@sylvestre sylvestre merged commit 1b4ef90 into uutils:main Jul 16, 2023
@sylvestre
Copy link
Contributor

Let's try
Thanks

@starccy
Copy link
Contributor Author

starccy commented Jul 16, 2023

And as I've mentioned above, the job will be triggered if we modify the shell script under the util dir. But in the current situation, the shellcheck/shfmt will complain about some scripts. Should I add an additional commit to fix them?

@sylvestre
Copy link
Contributor

yeah, please :)

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.

ci: Run shellcheck and shfmt on shell scripts in the util directory
3 participants