-
Notifications
You must be signed in to change notification settings - Fork 934
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
Add option to ignore shell library files for bash-exec #5254
Add option to ignore shell library files for bash-exec #5254
Conversation
bf06caa
to
aa414b1
Compare
aa414b1
to
b40a142
Compare
e6868a2
to
ac63fcd
Compare
Coming to think of it, I could probably test this with a separate Make target. Would that be an option? |
I see it also failed shfmt. I can figure that out. What's the preferred way here, add new commits and squash before merging, or always squash into a single commit? |
Thanks for this PR @bewuethr ! Either work. We would need to squash everything in a single commit anyway later, but while we're iterating on the PR, both are fine :) |
I think so :) Together with the new Make target, there's also the https://github.com/super-linter/super-linter/blob/main/test/run-super-linter-tests.sh script that we're using to reduce duplication in the Makefile. We didn't onboard existing targets yet, but it would be preferable to consider it for new targets. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR! A few small things to check.
:)
Shfmt errors: https://github.com/super-linter/super-linter/actions/runs/7863202043/job/21455961742?pr=5254#step:8:3858 (minor formatting issue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have addressed all comments, except the tests, which I'll work on next.
Regarding the Makefile, I don't think .phony
does anything; it has to be .PHONY
. Should I just do it for my target, update all targets, or should it be fixed in a separate PR?
Good catch. Can you please fix it for all the targets? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! We're almost there, nice work. Can you please also rebase on top of the latest main
so we can shorten the build time thanks to the build cache?
Thanks!
fe28586
to
c547730
Compare
Thanks @bewuethr, this is really awesome. There's only one small thing left to check, and we should be good to go. |
Commit linting fails because the test data doesn't pass ShellCheck, shfmt and other linters, what's a good way around that? |
I think there are two issues at play:
I didn't have a look at the first one, but you can solve the second by doing this:
By doing this you:
There's only one thing left to test: the |
Ok, for |
This is for the "expect failures" case:
|
Ah! I think I got the In super-linter/lib/functions/log.sh Line 62 in 947c92a
I don't think there's a good way to export Bash associative arrays, but you can solve this in several different ways:
Refactoring |
- Move log variables in log.sh to shorten linter.sh - Source log.sh as soon as possible, so we can use log functions and variables as soon as possible. - Validate 'boolean' log variables: LOG_DEBUG, LOG_VERBOSE, LOG_NOTICE, LOG_WARN, LOG_ERROR. - Move foreground color markers from an associative array (that we cannot export), inside the log function. This fixes an issue that we discovered while working on #5254 where color markers were not available to subprocesses. - Remove background color markers because we don't use them.
- Move log variables in log.sh to shorten linter.sh - Source log.sh as soon as possible, so we can use log functions and variables as soon as possible. - Validate 'boolean' log variables: LOG_DEBUG, LOG_VERBOSE, LOG_NOTICE, LOG_WARN, LOG_ERROR. - Move foreground color markers from an associative array (that we cannot export), inside the log function. This fixes an issue that we discovered while working on #5254 where color markers were not available to subprocesses. - Remove background color markers because we don't use them.
I saw you just merged the Edit: done! |
70b8138
to
78f03e1
Compare
Thanks @bewuethr ! Running workflows now. |
Ok, new failure in the
Can you please Add |
The two that are failing are:
|
|
@bewuethr I think we're good to go then. Anything else left to do? |
Just rebasing on latest trunk and squashing, I think! |
5e16d35
to
b4424f8
Compare
Ah! You just brought me back to SVN! :D |
Should I rebase on latest |
Introduce a new configuration variable, BASH_EXEC_IGNORE_LIBRARIES. If set to true, the behaviour of bash-exec is modified: if a shell file has a file extension and no shebang line, it is ignored, i.e., allowed to be non-executable. This allows files that are only every sourced from other shell files, acting as libraries and not executables, to have no executable bit set without failing the bash-exec linter.
b4424f8
to
d0d913d
Compare
@ferrarimarco Thank you for all your patience and responsiveness on this one! |
You're welcome! Thanks for your contributions! |
Proposed changes
Fix #5212
This introduces a new configuration variable,
BASH_EXEC_IGNORE_LIBRARIES
. If set totrue
, the behaviour of bash-exec is modified: if a shell file has a file extension and no shebang line, it is ignored, i.e., allowed to be non-executable. This allows files that are only every sourced from other shell files, acting as libraries and not executables, to have no executable bit set without failing the bash-exec linter.The rationale for looking at exactly the first two characters of the file and not skipping blank lines or similar is based on this Stack Overflow Q&A.
Readiness checklist
In order to have this pull request merged, complete the following tasks.
Pull request author tasks
I added the
Fix #ISSUE_NUMBER
label to the description of the pull request.Marked "upgrade instructions" as done, as none are required; I'll wait with squashing until this is ready to merge.
Super-linter maintainer tasks
breaking
if this change breaks compatibility with the previous released version.automation
,bug
,documentation
,enhancement
,infrastructure
.