Enable perlcritic checks in CI #31

b10n1k · 2024-02-07T11:42:09Z

Run perlcritic against all the perl git files.

https://progress.opensuse.org/issues/138416

.github/workflows/check_critic.yml

perlpunk · 2024-02-07T11:55:16Z

I think the git commit message checker can be replaced with what we have in openQA: https://github.com/os-autoinst/openQA/blob/master/.github/workflows/commit-message-checker.yml
I think it also has an exception for git subrepo commit messages.

b10n1k · 2024-02-07T12:00:59Z

i guess the errors are not by my PR??!

perlpunk · 2024-02-07T12:05:59Z

I think the isotovideo tests have been broken for a long time. I don't know anything about them, I think they were even there before I joined. Maybe someone else can say something about it, but I would say they can be ignored for this PR.
But the git commit message check can be fixed by what I said.

b10n1k · 2024-02-07T12:06:48Z

I think the git commit message checker can be replaced with what we have in openQA: https://github.com/os-autoinst/openQA/blob/master/.github/workflows/commit-message-checker.yml I think it also has an exception for git subrepo commit messages.

i will have to take a look on that

b10n1k · 2024-02-07T12:07:34Z

.github/workflows/check_critic.yml

+        run: |
+          git config --global --add safe.directory '*'
+          zypper -n install perl-Perl-Critic
+          perlcritic $(git ls-files -- '*.p[ml]')


i wonder whether i have to use https://github.com/os-autoinst/os-autoinst-common/blob/master/tools/perlcritic somehow here

Maybe @josegomezr or @okurz can answer that

perlcritic kinda "knows" what's a perl file, you can pass just the directories where it needs to be invoked.

In fact, the check is in the files you're pulling from os-autoinst-common. Use that as much as possible.

# [...] container: image: perldocker/perl-tester # this is now provided by os_autoinst dev container iirc steps: - uses: actions/checkout@v4 - run: ./tools/perlcritic --quiet # [...]

Martchus · 2024-02-07T17:18:48Z

Yes, the isotovideo checks were failing before as @perlpunk mentioned. And for the commit message check see her comment as well.

subrepo: subdir: "external/os-autoinst-common" merged: "e167bdf" upstream: origin: "https://github.com/os-autoinst/os-autoinst-common.git" branch: "master" commit: "e167bdf" git-subrepo: version: "0.4.6" origin: "???" commit: "???" Signed-off-by: ybonatakis <[email protected]>

Run perlcritic against all the perl git files. https://progress.opensuse.org/issues/138416 Signed-off-by: ybonatakis <[email protected]>

`.tidyallrc` doesnt work properly as a symlink, thus the new file is an actual copy from the external/os-autoinst-common. Signed-off-by: ybonatakis <[email protected]>

b10n1k · 2024-02-08T13:14:03Z

please do not merge yet. I am working on isotovideo failures

josegomezr · 2024-02-08T13:32:07Z

@b10n1k "github action example / test (pull_request)" and "check all test modules / test (pull_request)" always fail, don't ask me why.

#29 tackles a bit that so they are more native and run in a sensible manner in case you need another example around ;)

perlpunk · 2024-02-08T13:39:54Z

Yeah, you don't need to fix the isotovideo tests in this PR. sorry if that wasn't clear

Signed-off-by: ybonatakis <[email protected]>

isotovideo-action.yml uses a isotovideo version with already jq installed. fixed grep which excluded the expected output in success and changed logical and to logical or, which will reached when grep fails. Signed-off-by: ybonatakis <[email protected]>

Martchus

Nice that you've also been able to fix the isotovideo checks.

baierjan · 2024-02-08T15:29:10Z

.github/workflows/isotovideo-check-all-test-modules.yml

-        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'
+        env:
+          image: registry.opensuse.org/devel/openqa/containers/isotovideo:qemu-x86-jq
+          isotovideo: isotovideo qemu_no_kvm=1 casedir=/tests
+          jq_filter: jq .result testresults/result-*.json
+          err_msg: Test modules failed
+        run: docker run --rm -it -v .:/tests:Z --entrypoint '' $image /bin/sh -c '$isotovideo && $jq_filter | grep ok || (echo "$err_msg" && exit 1)'


How is this change related to perlcritic checks?

Is the switch from podman to docker intended? I thought podman is the preferred way.

Also, you probably do not want the exit in subshell and rather use grep ok || { echo "$err_msg" && exit 1; }

baierjan · 2024-02-08T15:36:33Z

.github/workflows/yaml-checks.yml

+      - name: Validate yamls
+        run: |
+          git config --global --add safe.directory '*'
+          yamllint --strict $(git ls-files "*.yml" "*.yaml" 2> /dev/null || find . -name '*.y*ml')


*.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.

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

baierjan · 2024-02-08T15:41:58Z

.github/workflows/isotovideo-action.yml

@@ -9,11 +9,9 @@ jobs:
      image: "registry.opensuse.org/devel/openqa/containers/isotovideo:qemu-x86-jq"
    steps:
      - uses: actions/checkout@v2


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

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

baierjan · 2024-02-08T15:43:43Z

.github/workflows/isotovideo-action.yml


      - 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)


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; }

baierjan · 2024-02-08T15:44:23Z

.github/workflows/isotovideo.yml

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


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

baierjan · 2024-02-08T15:45:11Z

.github/workflows/isotovideo-check-all-test-modules.yml

+          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)'


Again the subshell.

perlpunk · 2024-02-08T15:54:52Z

.github/workflows/isotovideo-action.yml


      - 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)


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.

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

perlpunk

There are already a lot of comments in this PR, so I suggest to keep the isotovideo checks out of here. It's unrelated and should be in its own PR.

okurz

Please split this PR into separate ones based on the commits you created. Consider https://progress.opensuse.org/projects/qa/wiki/Tools#Collaboration-best-practices

After that I would like to review the individual parts in more details

b10n1k · 2024-02-14T20:40:29Z

this is outdated. other PRs provided and merged

b10n1k changed the title ~~Update ci~~ Enable perlcritic checks in CI Feb 7, 2024

perlpunk reviewed Feb 7, 2024

View reviewed changes

.github/workflows/check_critic.yml Outdated Show resolved Hide resolved

b10n1k force-pushed the update_ci branch from 9167813 to ec2be12 Compare February 7, 2024 11:46

b10n1k commented Feb 7, 2024

View reviewed changes

b10n1k force-pushed the update_ci branch 2 times, most recently from b560bfc to e7d9ba1 Compare February 8, 2024 07:50

b10n1k added 3 commits February 8, 2024 08:51

Enable perlcritic checks in CI

2373ded

Run perlcritic against all the perl git files. https://progress.opensuse.org/issues/138416 Signed-off-by: ybonatakis <[email protected]>

Enable tidy checks in Ci

b9f34a5

`.tidyallrc` doesnt work properly as a symlink, thus the new file is an actual copy from the external/os-autoinst-common. Signed-off-by: ybonatakis <[email protected]>

b10n1k force-pushed the update_ci branch from e7d9ba1 to b9f34a5 Compare February 8, 2024 07:52

b10n1k marked this pull request as ready for review February 8, 2024 09:27

b10n1k force-pushed the update_ci branch 2 times, most recently from 0a11b01 to a4b69f8 Compare February 8, 2024 11:56

Martchus approved these changes Feb 8, 2024

View reviewed changes

perlpunk mentioned this pull request Feb 8, 2024

Allow 'git subrepo clone' in commit messages os-autoinst/os-autoinst-common#50

Merged

Enable yaml checks in CI

4d56c6d

Signed-off-by: ybonatakis <[email protected]>

b10n1k force-pushed the update_ci branch 6 times, most recently from f269a41 to 3946b97 Compare February 8, 2024 14:52

Fix isotovideo CI jobs

8e26f76

isotovideo-action.yml uses a isotovideo version with already jq installed. fixed grep which excluded the expected output in success and changed logical and to logical or, which will reached when grep fails. Signed-off-by: ybonatakis <[email protected]>

b10n1k force-pushed the update_ci branch from 3946b97 to 8e26f76 Compare February 8, 2024 14:53

b10n1k requested review from perlpunk and josegomezr February 8, 2024 15:06

Martchus approved these changes Feb 8, 2024

View reviewed changes

baierjan reviewed Feb 8, 2024

View reviewed changes

baierjan requested changes Feb 8, 2024

View reviewed changes

perlpunk reviewed Feb 8, 2024

View reviewed changes

okurz requested changes Feb 8, 2024

View reviewed changes

baierjan mentioned this pull request Feb 9, 2024

Update action version to 4 #33

Merged

b10n1k closed this Feb 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable perlcritic checks in CI #31

Enable perlcritic checks in CI #31

b10n1k commented Feb 7, 2024 •

edited

Loading

perlpunk commented Feb 7, 2024

b10n1k commented Feb 7, 2024

perlpunk commented Feb 7, 2024

b10n1k commented Feb 7, 2024

b10n1k Feb 7, 2024

perlpunk Feb 7, 2024

josegomezr Feb 7, 2024

Martchus commented Feb 7, 2024

b10n1k commented Feb 8, 2024

josegomezr commented Feb 8, 2024

perlpunk commented Feb 8, 2024

Martchus left a comment

baierjan Feb 8, 2024

baierjan Feb 8, 2024

perlpunk Feb 8, 2024

baierjan Feb 8, 2024

baierjan Feb 8, 2024

baierjan Feb 8, 2024

baierjan Feb 8, 2024

baierjan Feb 8, 2024

perlpunk Feb 8, 2024

baierjan Feb 8, 2024

perlpunk left a comment

okurz left a comment •

edited

Loading

b10n1k commented Feb 14, 2024

Enable perlcritic checks in CI #31

Enable perlcritic checks in CI #31

Conversation

b10n1k commented Feb 7, 2024 • edited Loading

perlpunk commented Feb 7, 2024

b10n1k commented Feb 7, 2024

perlpunk commented Feb 7, 2024

b10n1k commented Feb 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martchus commented Feb 7, 2024

b10n1k commented Feb 8, 2024

josegomezr commented Feb 8, 2024

perlpunk commented Feb 8, 2024

Martchus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perlpunk left a comment

Choose a reason for hiding this comment

okurz left a comment • edited Loading

Choose a reason for hiding this comment

b10n1k commented Feb 14, 2024

b10n1k commented Feb 7, 2024 •

edited

Loading

okurz left a comment •

edited

Loading