action #155188
closedopenQA Tests - coordination #96596: [qe-core][CI] CI/CD and Coding style improvements
Unify GitHub Actions for QA Projects - perltidy&perlcritic in openQA
Description
Motivation¶
See #138416
Acceptance criteria¶
- AC1: openQA inherit perltidy&perlcritic checks from os-autoinst-common
- AC2: Checks in openQA still run successfully on the master branch
Suggestions¶
- Consider the dependencycron updates to dependencies which currently runs in CircleCI
- Moving the deps to GHA would imply moving all tests to GHA
- Follow #155062 but for openQA
Updated by okurz 8 months ago
- Copied from action #155062: Unify GitHub Actions for QA Projects - perltidy in os-autoinst size:M added
Updated by okurz 8 months ago
- Copied to action #155191: Unify GitHub Actions for QA Projects - perlcritic in os-autoinst added
Updated by tinita 6 months ago
We were talking about this ticket in the estimation call.
The team understood the ticket as: Use the same github action as we use in os-autoinst-common to run perltidy https://github.com/os-autoinst/os-autoinst-common/blob/master/.github/workflows/perl-lint-checks.yml and perlcritic https://github.com/os-autoinst/os-autoinst-common/blob/master/.github/workflows/perl-critic.yml
For perlcritic that would work. Existing perlcritic rules are not changing over time depending on the module version (there might be rare expections, maybe fixed bugs in perl parsing).
For perltidy we are currently using CircleCI, and just changing this check to github would be "problematic", as we would lose our automatic update of formatting whenever perltidy is updated. Maybe we can live with that, I just want to mention it.
How the dependency cron is currently working¶
So we have the dependency cron running nightly on circleci.
https://github.com/os-autoinst/openQA/blob/master/tools/ci/prepare_dependency_pr.sh
It calls
https://github.com/os-autoinst/openQA/blob/master/tools/ci/build_dependencies.sh which installs openQA-devel and then does a diff against the installed packages before the installation.
Here is an example:
https://app.circleci.com/pipelines/github/os-autoinst/openQA/13102/workflows/42deeacd-3443-4aaf-9e80-8af40f06b7aa/jobs/122138
--- a/tools/ci/ci-packages.txt
+++ b/tools/ci/ci-packages.txt
@@ -238,7 +238,7 @@ perl-Perl-Critic-Policy-Variables-ProhibitLoopOnHash-0.008
perl-Perl-Critic-Pulp-99
perl-PerlIO-Layers-0.012
perl-PerlIO-utf8_strict-0.008
-perl-Perl-Tidy-20230912.0.0
+perl-Perl-Tidy-20240202.0.0
perl-Pod-MinimumVersion-50
perl-Pod-POM-2.01
perl-Pod-Spell-1.25
@@ -315,7 +315,7 @@ perl-XML-SAX-Expat-0.51
perl-XML-SemanticDiff-1.0007
perl-XML-Simple-2.24
perl-YAML-1.24
-perl-YAML-LibYAML-0.88
+perl-YAML-LibYAML-0.890.0
perl-YAML-PP-0.035
psmisc-23.0
python3-olefile-0.46
So it sees that packages were updated.
In that case it creates a PR with the updated packges: https://github.com/os-autoinst/openQA/pull/5505
Why? To actually run our testsuite with the updated packages to see if everything is still working.
That includes formatting changes by perltidy, as they could theoretically break tests.
As a result we see the PR, and if it passes, it will automatically be merged.
Now, if we didn't run the perltidy check in CircleCI anymore, but in GitHub, then the mentioned PR would actually fail, because the Github action would run an older version of perltidy from registry.opensuse.org/devel/openqa/containers/os-autoinst_dev which would possibly (not in all cases, some changes are backwards compatible) complain about the formatting changes.
If we move the perltidy check to github we would have to remove the perltidy automatic update handling from the dependency cron and have to make the formatting changes PR on our own, as soon as the package in registry.opensuse.org/devel/openqa/containers/os-autoinst_dev is updated and PRs start to fail.
But I don't see a big problem with just leaving things as they are.
In os-autoinst, we also didn't copy the os-autoinst-common check, but instead we have this: https://github.com/os-autoinst/os-autoinst/blob/master/.github/workflows/author-tests.yaml
The difference is, it's using cmake, and we couldn't agree on simply using a normal Makefile target for perltidy.
But what we could do for this ticket is linking the .perltidyrc to os-autoinst-common.
Updated by okurz 6 months ago
Now I get it, thx :)
But what we could do for this ticket is linking the .perltidyrc to os-autoinst-common.
Yes, that's what we should do. The GitHub action definition is simply enough that it's ok if we don't reuse it here. The main goal for the parent epic is anyway to provide good frameworks for test distributions, not necessarily openQA itself.
Updated by tinita 2 months ago
I could not resist: https://github.com/os-autoinst/openQA/pull/5838 Use new perltidy wrapper tools/tidyall
I was just looking for tidyall in openQA and didn't find it, only in os-autoinst.
I'm not sure why we split this out in this way and leave this ticket lying around for so long. It's not helpful if one project has /tools/tidyall
and the other one doesn't.
Updated by tinita 2 months ago · Edited
https://github.com/os-autoinst/openQA/pull/5838 merged.
https://github.com/os-autoinst/os-autoinst-common/pull/53 Fix Perl::Critic::Policy::HashKeyQuotes
Updated by tinita 2 months ago
I closed https://github.com/os-autoinst/openQA/pull/5841 , there was something wrong with circleci.
https://github.com/os-autoinst/openQA/pull/5843 Use perlcritic wrapper and plugins from common repo
Updated by tinita 2 months ago
https://github.com/os-autoinst/openQA/pull/5847 Remove duplicated use
https://github.com/os-autoinst/openQA/pull/5844 Fix some perlcritic complaints
Draft https://github.com/os-autoinst/os-autoinst-common/pull/54
Updated by tinita 2 months ago · Edited
- Status changed from In Progress to Feedback
https://github.com/os-autoinst/os-autoinst-common/pull/54 Fix and test perlcritic plugins (merged)
Updated by tinita 2 months ago
https://github.com/os-autoinst/openQA/pull/5843 is now passing and ready. This should conclude the ticket.