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

Enable perlcritic checks in CI #31

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .github/workflows/check_critic.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
name: perlcritic
on: [push, pull_request]

jobs:
perlcritic_checks:
runs-on: ubuntu-latest
container:
image: registry.opensuse.org/devel/openqa/containers/os-autoinst_dev
steps:
- uses: actions/checkout@v4
- name: Static analysis
run: |
git config --global --add safe.directory '*'
./external/os-autoinst-common/tools/perlcritic --quiet $(git ls-files -- '*.p[ml]')
6 changes: 2 additions & 4 deletions .github/workflows/isotovideo-action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,9 @@ jobs:
image: "registry.opensuse.org/devel/openqa/containers/isotovideo:qemu-x86-jq"
steps:
- uses: actions/checkout@v2
Copy link
Member

Choose a reason for hiding this comment

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

Can you also change the action to actions/checkout@v4?

Copy link
Member

Choose a reason for hiding this comment

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

That change should get rid of the deprecation warning seen in https://github.com/os-autoinst/os-autoinst-distri-example/actions/runs/7831480244

- name: install jq
run: zypper -n in jq

- name: Run isotovideo against test code
run: isotovideo qemu_no_kvm=1 casedir=.
run: isotovideo qemu_no_kvm=1 casedir=$(pwd)

- name: fail if any test module failed
run: jq .result testresults/result-*.json | grep -v ok && echo "Test modules failed" && exit 1
run: jq .result testresults/result-*.json | grep ok || (echo "Test modules failed" && exit 1)
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned elsewhere, you probably do not want subshell here, the exit will only exit the subshell.
grep ok || { echo "Test modules failed" && exit 1; }

Copy link
Contributor

Choose a reason for hiding this comment

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

Previously the grep was looking for any line not matching ok, so any failed test would make the workflow fail.
Now you are only ensuring that there is at least one line matching ok. So there could be failed modules and it would still succeed.
I don't know how many testmodules this workflow runs, but I think the logic should be kept as before, or look at @josegomezr ' PR.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks for commenting that. I also noticed that but forget to note down that grep -v ok is not the exact opposite of grep ok (I got sidetracked by the subshell 😄 ). I also vote for the previous logic; I believe the motivation was to avoid the non-zero exit code provided by grep on "success". We might want to use grep -v ok && { echo "Test modules failed" && exit 1; } || true

7 changes: 6 additions & 1 deletion .github/workflows/isotovideo-check-all-test-modules.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,9 @@ jobs:
steps:
- uses: actions/checkout@v2
- name: Run isotovideo against test code, fail if any test module failed
run: podman run --rm -it -v .:/tests:Z --entrypoint '' registry.opensuse.org/devel/openqa/containers/isotovideo:qemu-x86-jq /bin/sh -c 'isotovideo qemu_no_kvm=1 casedir=/tests && jq .result testresults/result-*.json | grep -v ok && echo "Test modules failed" && exit 1'
run: |
docker run --rm -v .:/tests:Z --entrypoint '' \
registry.opensuse.org/devel/openqa/containers/isotovideo:qemu-x86-jq /bin/sh \
-c 'isotovideo qemu_no_kvm=1 casedir=/tests \
&& jq .result testresults/result-*.json | grep ok \
|| (echo "Test modules failed" && exit 1)'
Copy link
Member

Choose a reason for hiding this comment

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

Again the subshell.

2 changes: 1 addition & 1 deletion .github/workflows/isotovideo.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@ jobs:
steps:
- uses: actions/checkout@v2
- name: Run isotovideo against test code in happy-path scenario
run: podman run --rm -it -v .:/tests:Z registry.opensuse.org/devel/openqa/containers/isotovideo:qemu-x86 qemu_no_kvm=1 casedir=/tests
run: docker run --rm -v .:/tests:Z registry.opensuse.org/devel/openqa/containers/isotovideo:qemu-x86 qemu_no_kvm=1 casedir=/tests
Copy link
Member

Choose a reason for hiding this comment

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

I understand the removal of -i and -t, why the podman -> docker change?

15 changes: 15 additions & 0 deletions .github/workflows/tidy_checks.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
name: tidyall
on: [push, pull_request]

jobs:
tidyall:
runs-on: ubuntu-latest
container:
image: registry.opensuse.org/devel/openqa/containers/os-autoinst_dev
steps:
- uses: actions/checkout@v4
- name: Static analysis
run: |
git config --global --add safe.directory '*'
./external/os-autoinst-common/tools/tidyall --check-only $(git ls-files)
15 changes: 15 additions & 0 deletions .github/workflows/yaml-checks.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
name: yamllint
on: [push, pull_request]

jobs:
yamllint:
runs-on: ubuntu-latest
container:
image: registry.opensuse.org/devel/openqa/containers/os-autoinst_dev
steps:
- uses: actions/checkout@v4
- name: Validate yamls
run: |
git config --global --add safe.directory '*'
yamllint --strict $(git ls-files "*.yml" "*.yaml" 2> /dev/null || find . -name '*.y*ml')
Copy link
Member

Choose a reason for hiding this comment

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

*.y*ml will match a lot of different things, maybe you want find . -name '*.yaml' -o -name '*.yml' to have closer match to the list used in git? Also 2> /dev/null might be good addition for the find as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

yamllint actually has a configuration file where you can specify the matching glob, and by default it will match .yaml and .yml.
You only need to pass it a directory name.
Please just use the same as in here https://github.com/os-autoinst/os-autoinst-common/blob/master/.github/workflows/yamllint.yml

Empty file added .gitignore
Empty file.
1 change: 1 addition & 0 deletions .perlcriticrc
1 change: 1 addition & 0 deletions .perltidyrc
3 changes: 3 additions & 0 deletions .tidyallrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[PerlTidy]
select = **/*.{pl,pm,t} tools/tidyall tools/perlcritic tools/update-deps
argv = --profile=$ROOT/.perltidyrc
Empty file added .yamlignore
Empty file.
1 change: 1 addition & 0 deletions .yamllint
12 changes: 12 additions & 0 deletions external/os-autoinst-common/.editorconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
[*]
indent_style = space
indent_size = 4
binary_next_line = true
switch_case_indent = true
space_redirects = true

[.bpan/**]
ignore = true

[*~]
ignore = true
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
---
name: 'Commit message check'

on:
workflow_call:
secrets:
accessToken:
required: true

jobs:
base-check-commit-message:
name: Check commit message
runs-on: ubuntu-latest
steps:
- name: Check subject beginning
uses: gsactions/commit-message-checker@v2
with:
pattern: '^([A-Z]|\S+:|git subrepo pull)'
flags: 'g'
error: 'The subject does not start with a capital or tag.'
excludeDescription: 'true'
excludeTitle: 'true'
checkAllCommitMessages: 'true'
accessToken: ${{ secrets.accessToken }}

- name: Check subject line length
uses: gsactions/commit-message-checker@v2
with:
pattern: '^.{1,72}(\n|$)'
flags: 'g'
error: 'The maximum subject line length of 72 characters is exceeded.'
excludeDescription: 'true'
excludeTitle: 'true'
checkAllCommitMessages: 'true'
accessToken: ${{ secrets.accessToken }}

- name: Check subject ending
uses: gsactions/commit-message-checker@v2
with:
pattern: '^.+(?<!\.)(\n|$)'
flags: 'g'
error: 'The subject cannot not end with a dot.'
excludeDescription: 'true'
excludeTitle: 'true'
checkAllCommitMessages: 'true'
accessToken: ${{ secrets.accessToken }}

- name: Check empty line
uses: gsactions/commit-message-checker@v2
with:
pattern: '^.*(\n\n|$)'
flags: 'g'
error: 'No newline between title and description.'
excludeDescription: 'true'
excludeTitle: 'true'
checkAllCommitMessages: 'true'
accessToken: ${{ secrets.accessToken }}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
name: 'Commit message check'

on:
pull_request:
push:
branches:
# we must not fix commit messages when they already reached master
- '!master'

jobs:
check-commit-message:
secrets:
accessToken: "${{ secrets.GITHUB_TOKEN }}"
uses: ./.github/workflows/base-commit-message-checker.yml
14 changes: 14 additions & 0 deletions external/os-autoinst-common/.github/workflows/perl-critic.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
name: 'Perl critic'

on: [push, pull_request]

jobs:
perl-critic-checks:
runs-on: ubuntu-latest
name: "Perlcritic"
container:
image: perldocker/perl-tester
steps:
- uses: actions/checkout@v4
- run: ./tools/perlcritic --quiet .
14 changes: 14 additions & 0 deletions external/os-autoinst-common/.github/workflows/perl-lint-checks.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
name: 'Perl static checks'

on: [push, pull_request]

jobs:
perl-lint-checks:
runs-on: ubuntu-latest
name: "Perltidy"
container:
image: registry.opensuse.org/devel/openqa/containers/os-autoinst_dev
steps:
- uses: actions/checkout@v4
- run: GITHUB_ACTIONS=1 ./tools/tidyall --check-only --all --quiet
15 changes: 15 additions & 0 deletions external/os-autoinst-common/.github/workflows/yamllint.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
name: 'YAML-lint'

on: [push, pull_request]

jobs:
yaml-lint:
runs-on: ubuntu-latest
name: "YAML-lint"
steps:
- uses: actions/checkout@v4
- uses: docker://registry.opensuse.org/home/okurz/container/containers/tumbleweed:yamllint
with:
entrypoint: yamllint
args: -c .yamllint --strict ./ --format github
1 change: 1 addition & 0 deletions external/os-autoinst-common/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
.tidyall.d/
12 changes: 12 additions & 0 deletions external/os-autoinst-common/.gitrepo
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
; DO NOT EDIT (unless you know what you are doing)
;
; This subdirectory is a git "subrepo", and this file is maintained by the
; git-subrepo command. See https://github.com/ingydotnet/git-subrepo#readme
;
[subrepo]
remote = https://github.com/os-autoinst/os-autoinst-common.git
branch = master
commit = e167bdf2bccb043d658da0cda7e63472ef6bff85
parent = 10683a4777aae56837fd0aaa789666914c5b71df
method = merge
cmdver = 0.4.6
29 changes: 29 additions & 0 deletions external/os-autoinst-common/.mergify.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
pull_request_rules:
- name: automatic merge
conditions:
- "#approved-reviews-by>=2"
- "#changes-requested-reviews-by=0"
# https://doc.mergify.io/examples.html#require-all-requested-reviews-to-be-approved
- "#review-requested=0"
- -label~=^acceptance-tests-needed|not-ready
- base=master
- "#check-failure=0"
- "#check-pending=0"
- linear-history
actions:
merge:
method: merge
- name: automatic merge on special label
conditions:
- -label~=^acceptance-tests-needed|not-ready
- "label=merge-fast"
- base=master
actions:
merge:
method: merge
- name: ask to resolve conflict
conditions:
- conflict
actions:
comment:
message: This pull request is now in conflicts. Could you fix it? 🙏
49 changes: 49 additions & 0 deletions external/os-autoinst-common/.perlcriticrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
theme = community + openqa
severity = 4
include = strict ValuesAndExpressions::ProhibitInterpolationOfLiterals

verbose = ::warning file=%f,line=%l,col=%c,title=%m - severity %s::[%p] %e\n

# == Perlcritic Policies
# -- Test::Most brings in strict & warnings
[TestingAndDebugging::RequireUseStrict]
equivalent_modules = Test::Most

[TestingAndDebugging::RequireUseWarnings]
equivalent_modules = Test::Most

# -- Avoid double quotes unless there's interpolation or a single quote.
[ValuesAndExpressions::ProhibitInterpolationOfLiterals]
allow_if_string_contains_single_quote = 1
severity = 3

# -- Prohibit deep nesting
[ControlStructures::ProhibitDeepNests]
severity = 4
add_themes = community
max_nests = 4

# == Community Policies
# -- Test::Most brings in strict & warnings
[Freenode::StrictWarnings]
extra_importers = Test::Most

# -- Test::Most brings in strict & warnings
[Community::StrictWarnings]
extra_importers = Test::Most

[Community::DiscouragedModules]
severity = 3

# Test modules have no package declaration
[Community::PackageMatchesFilename]
severity = 1

# == Custom Policies
# -- Useless quotes on hashes
[HashKeyQuotes]
severity = 5

# -- Superfluous use strict/warning.
[RedundantStrictWarning]
equivalent_modules = Test::Most
14 changes: 14 additions & 0 deletions external/os-autoinst-common/.perltidyrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# Workaround needed for handling non-ASCII in files.
# # See <https://github.com/houseabsolute/perl-code-tidyall/issues/84>.
--character-encoding=none
--no-valign
-l=160
-fbl # don't change blank lines
-fnl # don't remove new lines
-nsfs # no spaces before semicolons
-baao # space after operators
-bbao # space before operators
-pt=2 # no spaces around ()
-bt=2 # no spaces around []
-sbt=2 # no spaces around {}
-sct # stack closing tokens )}
3 changes: 3 additions & 0 deletions external/os-autoinst-common/.tidyallrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[PerlTidy]
select = **/*.{pl,pm,t} tools/tidyall tools/perlcritic tools/update-deps
argv = --profile=$ROOT/.perltidyrc
Empty file.
25 changes: 25 additions & 0 deletions external/os-autoinst-common/.yamllint
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
extends: default
ignore-from-file: [.gitignore, .yamlignore]

rules:
line-length:
max: 160
document-start: disable
indentation:
indent-sequences: true
spaces: 2

# Allows aligning subsequent lines with [] sequences
brackets:
min-spaces-inside: 0
max-spaces-inside: -1
commas:
max-spaces-after: -1

# Allows aligning key value pairs
colons:
max-spaces-after: -1

truthy:
allowed-values: ['true', 'false']
check-keys: false
21 changes: 21 additions & 0 deletions external/os-autoinst-common/LICENSE
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
MIT License

Copyright (c) 2019 openQA Development

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.
4 changes: 4 additions & 0 deletions external/os-autoinst-common/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
.PHONY: update-deps
update-deps:
tools/update-deps --cpanfile cpanfile

Loading
Loading