QA (public) » openQA Project (public)

@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?

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

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.

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.

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

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.

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

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

@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?

Sorry i found now
https://github.com/os-autoinst/scripts/pull/282/files
ignore

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.

Added in os-autoinst https://github.com/os-autoinst/os-autoinst/pull/2450

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?

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?

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.

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

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

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