Project

General

Profile

Actions

action #153793

closed

Ensure proper whitespace in os-autoinst scripts size:M

Added by okurz 9 months ago. Updated 8 months ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Feature requests
Target version:
Start date:
2024-01-17
Due date:
2024-02-02
% Done:

0%

Estimated time:

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
Actions #1

Updated by livdywan 9 months ago

  • Subject changed from Ensure proper whitespaces in os-autoinst scripts to Ensure proper whitespaces in os-autoinst scripts size:M
  • Description updated (diff)
  • Status changed from New to Workable
Actions #2

Updated by ybonatakis 9 months ago

  • Status changed from Workable to In Progress
  • Assignee set to ybonatakis
Actions #3

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

Actions #4

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?

Actions #5

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

Actions #6

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.

Actions #7

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.

Actions #8

Updated by tinita 9 months ago

  • Subject changed from Ensure proper whitespaces in os-autoinst scripts size:M to Ensure proper whitespace in os-autoinst scripts size:M
Actions #9

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]

[0] https://github.com/os-autoinst/scripts/pull/284

Actions #10

Updated by tinita 9 months ago

  • Subject changed from Ensure proper whitespaces in os-autoinst scripts size:M to Ensure proper whitespace in os-autoinst scripts size:M
  • Due date set to 2024-02-02
Actions #11

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

Actions #12

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.

Actions #13

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

Actions #14

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
Actions #15

Updated by ybonatakis 9 months ago

  • Status changed from Feedback to In Progress
Actions #16

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

[0] https://github.com/os-autoinst/scripts/pull/285

Actions #17

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?

Actions #19

Updated by ybonatakis 9 months ago

  • Status changed from Feedback to In Progress
Actions #20

Updated by ybonatakis 9 months ago

  • Status changed from In Progress to Feedback
Actions #23

Updated by ybonatakis 9 months ago

  • Status changed from In Progress to Feedback
Actions #24

Updated by okurz 9 months ago

  • Status changed from Feedback to In Progress

For os-autoinst the PR was merged. You can now add the call to shfmt in CI rules

Actions #26

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?

Actions #27

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?

Actions #28

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.

Actions #29

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

Actions #30

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

Actions #31

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

Actions #32

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.

Actions #33

Updated by ybonatakis 8 months ago

  • Status changed from In Progress to Feedback
Actions #34

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)

Actions

Also available in: Atom PDF