Reusable Workflow Experiment: Upstreaming commit message checks #28

josegomezr · 2023-10-24T12:11:37Z

Bringing checks from Update commit-message-checker & add extra rule for subject lines openQA#5343.
Setting up reusable workflows 0.

This is an experiment 🥼

First Error:

There's a successful schedule of the check: https://github.com/os-autoinst/os-autoinst-common/actions/runs/6626891884

Proposed Agreement:

Define all checks as base-%{check-name}.yml into .github/workflows.
Use them in %{check-name}.yml in .github/workflows.
Make PR's in each derived repo so the workflow is used instead of re-defined. See the snippet below:

Reusable Workflow usage on downstream repos:

---
name: 'Commit Message Check'

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

jobs:
  call-check-commit-message:
    uses: os-autoinst/os-autoinst-common./.github/workflows/base-commit-message-checker.yml@master
    secrets:
      accessToken: "${{ secrets.GITHUB_TOKEN }}"

mergify · 2023-10-24T12:26:07Z

This pull request is now in conflicts. Could you fix it? 🙏

okurz

Please incorporate the changes from #29 . Idea looks nice

josegomezr · 2023-10-24T13:04:47Z

The expressions are already present here (rebased after #29 got merged).

It'd be great if we can come up with a list of general Checks to add them here at once and go then over the downstream repositories referring to the checks defined here.

okurz · 2023-10-24T13:08:12Z

It'd be great if we can come up with a list of general Checks to add them here at once and go then over the downstream repositories referring to the checks defined here.

Well, I guess that's the idea of the PR and I like the idea of the PR or what do you mean?

josegomezr · 2023-10-24T13:22:44Z

what do you mean?

What checks can be generalized so they apply to all repos?

Like the commit message one is a simple task, that's even agnostic of the language.

How about

Perl/Python/JS/%lang% code style/lint checks?
Spell Checks?
%insert here a check%

?

Addenda:

Could this repo host also linter rules so they can downstream transparently to the dependent repos?

perlpunk · 2023-10-24T15:53:20Z

There was an attempt to also put tools/tidy in here, but it's not that trivial because of the way os-autoinst-distri-opensuse is using this script: https://progress.opensuse.org/issues/110142
It's solvable, but we didn't have time.

.github/workflows/base-commit-message-checker.yml

foursixnine · 2023-10-24T20:19:18Z

.perltidyrc

+# # See <https://github.com/houseabsolute/perl-code-tidyall/issues/84>.
+--character-encoding=none
+--no-valign
+# TBD: openqa defines: -l=120 vs -l=160 in os-autoinst-distri-opensuse


Lets stick to 160, we're not in the 80's anymore, screens are bigger and I don't expect people to do a proper review using their phones.

Despite hearing that argument very often, the restricted line length is not about the screen width, but also about horizontal eye movement, and additionally side by side diff views are harder with longer lines.
But in my team I'm in the minority, most prefer longer lines anyway ;-)

.perltidyrc

foursixnine · 2023-10-24T20:20:39Z

bin/perlcritic

+# This script will either be called directly from this project or symlinked from
+# downstream projects.
+
+OS_AUTOINST_COMMONS_DIR=$(dirname $(dirname $(readlink -e "$0")))


why the double dirname?

readlink $0 will resolve the path of the specific file including symlinks.

that's a/b/c/os-autoinst-common/bin/perlcritic but we need what's on a/b/c/os-autoinst-common/lib/perlcritic.

First dirname deletes perlcritic, the second deletes bin and we end up with the desired path.

See:

dirname a/b/c/os-autoinst-common/bin/perlcritic #> a/b/c/os-autoinst-common/bin dirname a/b/c/os-autoinst-common/bin #> a/b/c/os-autoinst-common

foursixnine · 2023-10-24T20:26:55Z

bin/tidyall

+        printf STDERR "Please install the appropriate version of perltidy.\n";
+        printf STDERR "If you want to proceed anyways, re run with --force flag.\n";


Maybe this can be formatted to make it pop up on GH workflows (https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-an-error-message)

This is a local-only tool, tools/tidy is not run in any workflow that I can find, perlcritic does through run_unit_tests.sh and Makefile targets.

The idea is that is a transparent wrapper over tidyall so no special text processing is needed on ever divergent bash scripts.

josegomezr · 2023-10-24T21:05:34Z

There was an attempt to also put tools/tidy in here, but it's not that trivial because of the way os-autoinst-distri-opensuse is using this script: https://progress.opensuse.org/issues/110142

In this implementation, how Perl::Tidy is provisioned (cpanfile or RPM) is irrelevant, as long as the correct version is installed (that is to say: perl knows about it) then bin/tidyall works as expected.

When it's not installed then:

Can't locate Perl/Tidy.pm in @INC (you may need to install the Perl::Tidy module) (@INC contains: /usr/lib/perl5/site_perl/5.26.1/x86_64-linux-thread-multi /usr/lib/perl5/site_perl/5.26.1 /usr/lib/perl5/vendor_perl/5.26.1/x86_64-linux-thread-multi /usr/lib/perl5/vendor_perl/5.26.1 /usr/lib/perl5/5.26.1/x86_64-linux-thread-multi /usr/lib/perl5/5.26.1 /usr/lib/perl5/site_perl) at bin/tidyall line 4.
BEGIN failed--compilation aborted at bin/tidyall line 4.

If a more graceful error is expected, maybe beginning with:

eval { require Perl::Tidy; };

if($@){
    die "Perl::Tidy is not installed";
}

Would allow to report more gracefully that perltidy is not installed?

I lean towards avoiding it. The setup guides should get each developer to a point where all perl dependencies are installed properly.

perlpunk · 2023-10-24T21:58:47Z

bin/tidyall

@@ -0,0 +1,32 @@
+#!/usr/bin/env perl


tidyall is a script provided by the perl module Code::TidyAll, so I think creating a script with the same name is unfortunate.
Like I said, tidyall provides tidy tools for several languages/tools, including perltidy.

We use it mainly because it has a caching functionality, so it is faster to use locally.
In CI we are using https://github.com/os-autoinst/openQA/blob/master/t/00-tidy.t and https://github.com/os-autoinst/os-autoinst/blob/master/xt/00-tidy.t (and yes, it's unfortunate that they are different :)

I'm very open to suggestions on naming, the goal with this is to minimize scattering across repos and differences.

from your examples I see two ways of approaching linter checks:

Run it as a test in t/ (which I guess prove runs).

Run it as a standalone command (which is what the openQA version is doing).

I simplified the latter, as I see there are may versions of the same script. But I don't know if either of the versions is more or less beneficial.

I guess it's also team agreement on which strategy to pick.

IMHO: I lean for the standalone command because it looks more similar to how other stacks work:

in Ruby you'd install dependencies & bundle exec rubocop

in Python you'd install dependencies, virtualenv [with your favourite virtualenv tool] & {black|flake8|pylint}.

in JS you install dependencies & eslint.

But I'm also wondering about other things.
I use tools/tidy -o to only tidy changed files.
Also it has a check-only mode.

And tools for development shouldn't be in bin/, as this is usually the directory that contains scripts that are packaged.

perlpunk · 2023-10-24T22:01:42Z

.perltidyrc

@@ -0,0 +1,18 @@
+# Workaround needed for handling non-ASCII in files.


See https://github.com/os-autoinst/os-autoinst/blob/master/xt/00-tidy.t vs. https://github.com/os-autoinst/openQA/blob/master/.perltidyrc

So we would have to decide about which config we want to use in the future.

Looks like tidyall_ok uses the default config file:

https://github.com/houseabsolute/perl-code-tidyall/blob/10425fb8e720c06f96d3935bc98b2955fc3e3294/lib/Test/Code/TidyAll.pm#L23-L29

looks like it'd be a matter of providing this one as a symlink and that would be it ?

Yeah, the point is, if we use the symlink to this one here, we would first need to decide which of the two different configs we want.
I think it's better if we do that first by providing single PRs to both repos with the suggested new config and look at the tidy changes, so we can decide.

perlpunk · 2023-10-24T22:06:25Z

Can you please make seperate pull requests for all these concerns? The sharing of the workflow was a good idea and seems to work. The other things are a bit more complicated because some things are also used from os-autoinst-distri-opensuse.
Maybe you want to join our daily Team meeting tomorrow at 10:30 to discuss things?

okurz

I like the overall approach very much but I don't think it's feasible to bring in all those rules at once. Let's please start with the common ground of git commit message check and see that this is applied in all relevant downstream projects first. Then we can extend to tidy+perlcritic, etc. You can keep this PR as is and split out the git commit message check for a start into a separate PR and then rebase here, ok?

mergify · 2023-10-25T10:31:48Z

This pull request is now in conflicts. Could you fix it? 🙏

- Bringing checks from os-autoinst/openQA#5343. - Setting up reusable workflows [0]. [0]: https://docs.github.com/en/actions/using-workflows/reusing-workflows Reusable Workflow Experiment: Upstreaming commit message checks - Bringing checks from os-autoinst/openQA#5343. - Setting up reusable workflows [0]. [0]: https://docs.github.com/en/actions/using-workflows/reusing-workflows

josegomezr · 2023-10-25T10:42:56Z

You can keep this PR as is and split out the git commit message check for a start into a separate PR and then rebase here, ok?

Done, took a bit of problems with my local copy but it's there.

There's one thing that doesn't look quite right: the workflow for commit needs an approval. That's a fairly trivial check that can run automatically. Any idea how to make that? Nothing in the YAML's says anything about manual workflows

perlpunk · 2023-10-25T11:18:14Z

For everyone who does their first contribution on a github repo, workflow runs need to be approved. This was introduced by Github a while ago to prevent crypto miners from using resources.
So once your first PR has been merged, workflows will run automatically.
I will approve the workflow run manually for now.

josegomezr force-pushed the experiment_reusable_workflows branch 3 times, most recently from e766cc0 to 96cf0da Compare October 24, 2023 12:25

josegomezr force-pushed the experiment_reusable_workflows branch 5 times, most recently from d0ebcb5 to a05602b Compare October 24, 2023 12:36

okurz requested changes Oct 24, 2023

View reviewed changes

perlpunk reviewed Oct 24, 2023

View reviewed changes

.github/workflows/base-commit-message-checker.yml Outdated Show resolved Hide resolved

foursixnine reviewed Oct 24, 2023

View reviewed changes

.perltidyrc Outdated Show resolved Hide resolved

foursixnine reviewed Oct 24, 2023

View reviewed changes

perlpunk reviewed Oct 24, 2023

View reviewed changes

okurz requested changes Oct 25, 2023

View reviewed changes

josegomezr force-pushed the experiment_reusable_workflows branch from 6677812 to f06acfe Compare October 25, 2023 10:31

josegomezr force-pushed the experiment_reusable_workflows branch 3 times, most recently from e0f2008 to 797d222 Compare October 25, 2023 10:35

josegomezr force-pushed the experiment_reusable_workflows branch from 797d222 to d23cda4 Compare October 25, 2023 10:38

perlpunk approved these changes Oct 25, 2023

View reviewed changes

okurz approved these changes Oct 25, 2023

View reviewed changes

mergify bot merged commit 0cfcdb3 into os-autoinst:master Oct 25, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reusable Workflow Experiment: Upstreaming commit message checks #28

Reusable Workflow Experiment: Upstreaming commit message checks #28

josegomezr commented Oct 24, 2023 •

edited

Loading

mergify bot commented Oct 24, 2023

okurz left a comment

josegomezr commented Oct 24, 2023

okurz commented Oct 24, 2023

josegomezr commented Oct 24, 2023 •

edited

Loading

perlpunk commented Oct 24, 2023

foursixnine Oct 24, 2023

perlpunk Oct 24, 2023 •

edited

Loading

foursixnine Oct 24, 2023

josegomezr Oct 24, 2023

foursixnine Oct 24, 2023

josegomezr Oct 24, 2023

josegomezr commented Oct 24, 2023

perlpunk Oct 24, 2023 •

edited

Loading

josegomezr Oct 24, 2023

perlpunk Oct 24, 2023

perlpunk Oct 24, 2023

josegomezr Oct 24, 2023

perlpunk Oct 24, 2023

perlpunk commented Oct 24, 2023 •

edited

Loading

okurz left a comment

mergify bot commented Oct 25, 2023

josegomezr commented Oct 25, 2023

perlpunk commented Oct 25, 2023 •

edited

Loading

		printf STDERR "Please install the appropriate version of perltidy.\n";
		printf STDERR "If you want to proceed anyways, re run with --force flag.\n";

		@@ -0,0 +1,18 @@
		# Workaround needed for handling non-ASCII in files.

Reusable Workflow Experiment: Upstreaming commit message checks #28

Reusable Workflow Experiment: Upstreaming commit message checks #28

Conversation

josegomezr commented Oct 24, 2023 • edited Loading

mergify bot commented Oct 24, 2023

okurz left a comment

Choose a reason for hiding this comment

josegomezr commented Oct 24, 2023

okurz commented Oct 24, 2023

josegomezr commented Oct 24, 2023 • edited Loading

perlpunk commented Oct 24, 2023

Choose a reason for hiding this comment

perlpunk Oct 24, 2023 • edited Loading

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

josegomezr commented Oct 24, 2023

perlpunk Oct 24, 2023 • edited Loading

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 commented Oct 24, 2023 • edited Loading

okurz left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 25, 2023

josegomezr commented Oct 25, 2023

perlpunk commented Oct 25, 2023 • edited Loading

josegomezr commented Oct 24, 2023 •

edited

Loading

josegomezr commented Oct 24, 2023 •

edited

Loading

perlpunk Oct 24, 2023 •

edited

Loading

perlpunk Oct 24, 2023 •

edited

Loading

perlpunk commented Oct 24, 2023 •

edited

Loading

perlpunk commented Oct 25, 2023 •

edited

Loading