action #153793
closedEnsure proper whitespace in os-autoinst scripts size:M
0%
Description
Motivation¶
We want consistent whitespace indentation in all our scripts.
https://github.com/os-autoinst/scripts/pull/278#discussion_r1455812690
Acceptance Criteria¶
- AC1: Pull requests fail when there are tabs used for indentation in shellscript code
- AC2: Exceptions where tabs are required are considered
Suggestions¶
- Research how to check for spaces using shellcheck or other linters and extend the CI accordingly, if needed with exceptions
- Ensure the according checks are implemented
- Consider using the same checks in our other usual repos when or where we also check shell script files
Updated by ybonatakis 9 months ago
- Status changed from Workable to In Progress
- Assignee set to ybonatakis
Updated by openqa_review 9 months ago
- Due date set to 2024-02-02
Setting due date based on mean cycle time of SUSE QE Tools
Updated by okurz 9 months ago
@ybonatakis I understood you have not found any good existing applications ensuring a proper whitespace indentation rules in shell script. I suggest you do not write your own script. Before we consider that we should sort out #138416
Anybody else able to find good upstream solutions to include?
Updated by livdywan 9 months ago
Anybody else able to find good upstream solutions to include?
https://github.com/mvdan/sh maybe? suggested in workadventu.re
https://www.shellcheck.net/wiki/ seems to have nothing generic enough wrt spaces and indentation, besides here docs
Updated by mkittler 9 months ago
I tried https://github.com/mvdan/sh. It is packaged for Tumbleweed and it can do the formatting via shfmt … --indent 4 filename…
. However, it would also introduce many other changes even in simple scripts (like tools/tidy
) and I couldn't find a flag to turn that off. So I'm not sure whether we want to use it.
Updated by tinita 9 months ago
I tried https://github.com/mvdan/sh as well, on the scripts repo, and I quite like it actually.
Of course initially there would be many changes, mostly because we have mixed (2 and 4) indentation though.
Updated by ybonatakis 9 months ago
- Subject changed from Ensure proper whitespace in os-autoinst scripts size:M to Ensure proper whitespaces in os-autoinst scripts size:M
- Due date deleted (
2024-02-02) - Status changed from In Progress to Feedback
I created the PR with the bash script which i added in the Github workflow[0]
Updated by ybonatakis 9 months ago
A list of files which could change as it was reported by https://github.com/mvdan/sh
Note: i think we can ignore .bpan
❯ docker run --rm -u "$(id -u):$(id -g)" -v "$PWD:/mnt" -w /mnt bashlinter -l .
.bpan/.bpan/lib/getopt.bash:336:20: = must follow a name
.bpan/share/template/library.bash:1:19: a command can only contain words and redirects; encountered (
.bpan/share/template/rc.sh:70:12: % must follow an expression
_common
backlog-set-due-date
chat_notify.sh
monitor-openqa_job
obs-check-devel-openqa-leap-extra-dependencies
openqa-advanced-retrigger-jobs
openqa-incompletes-stats
openqa-investigate
openqa-investigate-multi
openqa-label-known-issues
openqa-label-known-issues-and-investigate-hook
openqa-label-known-issues-multi
openqa-monitor-incompletes
openqa-monitor-investigation-candidates
openqa-query-for-job-label
os-autoinst-obs-auto-submit
reboot-stability-check
steps.sh
tools/tab_checker
trigger-openqa_in_openqa
trigger-pflash-test
I tried also other parameters like -i 2 -ci -bn -l .
Results where quite similar. For explanation of the params see https://github.com/mvdan/sh/blob/master/cmd/shfmt/shfmt.1.scd
Updated by tinita 9 months ago · Edited
How about making a PR or two where we all can see the diff?
Maybe a PR with 2 spaces indentation and one with 4, then we can decide if we want this and which?
You can use git ls-files
to avoid running it on .bpan etc.
This worked well here:~
shfmt -i 4 -w -bn $(git ls-files)
edit: nevermind, this will also try to lint non-shell files. not sure how certain files or dirs can be ignored yet.
Updated by tinita 9 months ago
I tried https://github.com/mvdan/sh/blob/master/cmd/shfmt/shfmt.1.scd#examples
% cat .editorconfig
[.bpan/**]
ignore = true
% shfmt -i 4 -w -bn -l .
.bpan/.bpan/lib/getopt.bash:336:20: = must follow a name
.bpan/share/template/library.bash:1:19: a command can only contain words and redirects; encountered (
.bpan/share/template/rc.sh:70:12: % must follow an expression
_common
...
so the ignore
setting - ironically - is ignored .
According to zypper, it's version 3.5.1, and I don't see anything regading "ignore" in the last releases: https://github.com/mvdan/sh/releases
Updated by tinita 9 months ago · Edited
ok, I got something working:
% cat .editorconfig
[*]
indent_style = space
indent_size = 4
binary_next_line = true
[.bpan/**]
ignore = true
% shfmt -w -l .
_common
backlog-set-due-date
chat_notify.sh
monitor-openqa_job
obs-check-devel-openqa-leap-extra-dependencies
openqa-investigate
openqa-label-known-issues
openqa-label-known-issues-and-investigate-hook
os-autoinst-obs-auto-submit
reboot-stability-check
steps.sh
trigger-openqa_in_openqa
Updated by ybonatakis 9 months ago
- Status changed from In Progress to Feedback
Here is the work for shfrm [0] with CI setup and files which had to be modified.
I still need to consider moving it to https://github.com/os-autoinst/os-autoinst-common but it can be in another step? idk exactly how git-subrepo
should work to apply this in CI of another repo
Updated by ybonatakis 9 months ago
@okurz I dont see the use of https://github.com/os-autoinst/os-autoinst-common on the repo. I guess you want another ticket for that or whats your plan?
Updated by ybonatakis 9 months ago
Sorry i found now
https://github.com/os-autoinst/scripts/pull/282/files
ignore
Updated by ybonatakis 9 months ago
- Status changed from In Progress to Feedback
Following up, the next repos has been updated to use shfmt
https://github.com/os-autoinst/os-autoinst/pull/2448
https://github.com/os-autoinst/openQA/pull/5440/files
.editorconfig already in os-autoinst-common https://github.com/os-autoinst/os-autoinst-common/pull/37
Updated by okurz 9 months ago
- Status changed from Feedback to In Progress
https://github.com/os-autoinst/os-autoinst-common/pull/37 is merged.
There are some open points in https://github.com/os-autoinst/os-autoinst/pull/2448 and https://github.com/os-autoinst/openQA/pull/5440 to look into.
And there is https://github.com/os-autoinst/os-autoinst-common/pull/39
Updated by okurz 9 months ago
I created
- https://github.com/os-autoinst/os-autoinst/pull/2449
- https://github.com/os-autoinst/openQA/pull/5444
to add shfmt to dependencies first.
Updated by ybonatakis 9 months ago
- Status changed from In Progress to Feedback
https://github.com/os-autoinst/os-autoinst/pull/2448
https://github.com/os-autoinst/openQA/pull/5440
are still open but i believe all the comments were addressed.
Updated by ybonatakis 9 months ago
Added in os-autoinst https://github.com/os-autoinst/os-autoinst/pull/2450
Updated by ybonatakis 8 months ago
So after spending some time with the Ci failure on the javascript checks, I found the prettier plugin uses the .editorconfig. @mkittler found some conditions which could/should work but everything we tried so far didnt work.
Other solutions we tried
- create a config file and pass it to eslint in the package.json. This is obviously didnt work because the settings of the plugin takes precedence.
- move the eslintrc config to the assets/javascripts
As the solution looks tricky, the easiest work around is to change the indent_size
from 4 to 2. That means that I will have to update again all the relevant repos which merged recently. wdyt?
Updated by jbaier_cz 8 months ago
ybonatakis wrote in #note-26:
So after spending some time with the Ci failure on the javascript checks, I found the prettier plugin uses the .editorconfig. @mkittler found some conditions which could/should work but everything we tried so far didnt work.
Other solutions we tried
- create a config file and pass it to eslint in the package.json. This is obviously didnt work because the settings of the plugin takes precedence.
- move the eslintrc config to the assets/javascripts
As the solution looks tricky, the easiest work around is to change the
indent_size
from 4 to 2. That means that I will have to update again all the relevant repos which merged recently. wdyt?
Do I get it correctly that the problem is in
[*]
indent_style = space
indent_size = 4
binary_next_line = true
which is interpreted by the javascript tools? Wouldn't it help to add
[*.js]
ignore = true
or something similar?
Updated by livdywan 8 months ago
As the solution looks tricky, the easiest work around is to change the
indent_size
from 4 to 2. That means that I will have to update again all the relevant repos which merged recently. wdyt?
Feedback in jitsi was in favor of 4 spaces. Using Tina's approach seems the easiest option. Put it in a script, or makefile. If it can't be shared so be it.
Updated by ybonatakis 8 months ago
jbaier_cz wrote in #note-27:
ybonatakis wrote in #note-26:
So after spending some time with the Ci failure on the javascript checks, I found the prettier plugin uses the .editorconfig. @mkittler found some conditions which could/should work but everything we tried so far didnt work.
Other solutions we tried
- create a config file and pass it to eslint in the package.json. This is obviously didnt work because the settings of the plugin takes precedence.
- move the eslintrc config to the assets/javascripts
As the solution looks tricky, the easiest work around is to change the
indent_size
from 4 to 2. That means that I will have to update again all the relevant repos which merged recently. wdyt?Do I get it correctly that the problem is in
[*] indent_style = space indent_size = 4 binary_next_line = true
which is interpreted by the javascript tools? Wouldn't it help to add
[*.js] ignore = true
or something similar?
or that. i think it worth a try
Updated by ybonatakis 8 months ago
ybonatakis wrote in #note-29:
jbaier_cz wrote in #note-27:
ybonatakis wrote in #note-26:
So after spending some time with the Ci failure on the javascript checks, I found the prettier plugin uses the .editorconfig. @mkittler found some conditions which could/should work but everything we tried so far didnt work.
Other solutions we tried
- create a config file and pass it to eslint in the package.json. This is obviously didnt work because the settings of the plugin takes precedence.
- move the eslintrc config to the assets/javascripts
As the solution looks tricky, the easiest work around is to change the
indent_size
from 4 to 2. That means that I will have to update again all the relevant repos which merged recently. wdyt?Do I get it correctly that the problem is in
[*] indent_style = space indent_size = 4 binary_next_line = true
which is interpreted by the javascript tools? Wouldn't it help to add
[*.js] ignore = true
or something similar?
or that. i think it worth a try
nope. and maybe it will be wrong as i would expect to ignore all the js and no checks would happen at the end
Updated by ybonatakis 8 months ago
- Status changed from In Progress to Feedback
CI looks finally good.
Note we do not use .editorconfig from os-autoinst-common for the bash scripts in openQA as we did to other repos
Updated by okurz 8 months ago
- Status changed from Feedback to In Progress
https://github.com/os-autoinst/os-autoinst/pull/2450 has more review comments to attend.
Updated by ybonatakis 8 months ago
- Status changed from Feedback to Resolved
writeup:
we integrate https://github.com/mvdan/sh to openQA, os-autoinst and scripts repos.
We implemented and added the config file in os-autoinst-common which os-autoinst and scripts retrieve.
However .editorconfig
conflicts with prettier
plugin in openQA so we the cli invocation directly on openQA. (shfmt -d -i 4 -bn -ci -sr)
Updated by okurz 8 months ago
Ok, looks good. I could also find the check in https://app.circleci.com/pipelines/github/os-autoinst/openQA/12865/workflows/a872d811-ef2e-4b68-9e88-fbcbef242cca/jobs/119908/parallel-runs/0/steps/0-107?invite=true#step-107-11156_11 for openQA and https://github.com/os-autoinst/os-autoinst/actions/runs/7718504678/job/21039916953#step:3:609 for os-autoinst