Re: [PATCH 1/1] ci: new github-action for git-l10n code review

Hi,

On Mon, 23 Aug 2021, Jiang Xin wrote:

From: Jiang Xin <redacted>

Git l10n uses github pull request for code review. A helper program
"git-po-helper" can be used to check typos in ".po" files, validate
syntax, and check commit message. It would be convenient to integrate
this helper program to CI and add comments in pull request.

The new github-action workflow is added in ".github/workflows/l10n.yml",
which is disabled by default. To turn it on for the git-l10n related
repositories, such as "git-l10n/git-po", we can add a new branch named
"ci-config" and create a simple shell script at "ci/config/allow-l10n"
in this branch.

The new l10n workflow listens to two types of github events: push and
pull_request.

For a push event, it will scan commits one by one. If a commit does not
look like a l10n commit (no file in "po/" has been changed), it will
immediately fail without checking for further commits. While for a
pull_request event, all new introduced commits will be scanned.

"git-po-helper" will generate two kinds of suggestions, errors and
warnings. A l10n contributor should try to fix all the errors, and
should pay attention to the warnings. All the errors and warnings will
be reported in the last step of the l10n workflow as two message groups.
For a pull_request event, will create additional comments in pull
request to report the result.

It is a good idea to automate this.

I am a bit concerned that the `ci-config` approach, even if we use it in
the Git project itself, is quite cumbersome to use, though. So I hope that
we can find an alternative solution.

One such solution could be to make the `git-po-helper` job contingent on
part of the repository name. For example:

  git-po-helper:
    if: endsWith(github.repository, '/git-po')
    [...]

would skip the job unless the target repository's name is `git-po`.

A couple more questions/suggestions are below:

There is code below to specifically fetch the base commit, but setting the
`fetch-depth` to zero means to do a full clone anyway.

So maybe the `git fetch` code below should be dropped, or the
`fetch-depth` override?

Since we cannot know how deep we need to fetch, though, I figure that it
would be indeed better to have a non-shallow clone (and a code comment
would help clarify this).

An even better alternative would be a partial clone, of course, but I fear
that there is no convenient way yet to configure `actions/checkout` to do
so.

Since this is a one-liner, it would probably make sense to avoid that `|`
continuation.

This would probably read better as:

		case "$commit_from" in
		*[^0]*|'') ;; # not all zeros
		*)
			[...]
			;;
		esac

But we might not need it anyway. See the comment on the `git fetch`
command below.

As pointed out above, we cannot know how many commits we need to fetch.
Therefore, I would suggest to simply drop this `git fetch`.

What does the `--github-action` option do? Might be good to document this
here.

Do we really need this `has_error_msg` variable? It would be much easier
to test the condition `env.ERROR_MSG != ''` in subsequent steps, and that
would already do the job, without having to go through output variables.

This is a bit dangerous. First of all, how can you be sure that
`git-po-helper.out` does not contain lines that consist of the letters
`EOF` (and would therefore interfere with the here-doc)?

Second, isn't it conceivable to have an `error:` line which contains the
characters `WARNING` too? That line would be filtered out...

Third, can `git-po-helper` be convinced _not_ to print color codes? The
output was redirected into a file, after all, and it does not go to a
terminal...

This requires the `GITHUB_TOKEN` to have write permissions, which I would
highly recommend not to require.

Instead, it would probably make more sense to keep the output in the logs
of the workflow run.

This would be easier to read and maintain if it was upgraded to an `if:`
condition:

    - name: Report errors
      if: step.check-commits.outputs.has_error_msg = "yes"
      run: |
        [...]

And then do the same for warnings.

Also, it would be more natural if the `Run git-po-helper` step was allowed
to fail when `git-po-helper` outputs errors. You would then have to use
this conditional in the `Report errors` step:

      if: failure() && step.check-commits.outputs.has_error_msg = "yes"

(compare to how Git's `CI/PR` workflow prints errors:
https://github.com/git/git/blob/v2.33.0/.github/workflows/main.yml#L120).

For the `Report warnings` step, you would however have to use something
slightly less intuitive:

      if: (success() || failure()) && step.check-commits.outputs.has_warning_msg = "yes"

Finally, I _do_ think that this line would be easier to read like this,
while basically doing the same thing but with less effort (because the
outputs would no longer have to be set):

      if: (success() || failure()) && env.WARNING_MSG != ""

Ciao,
Dscho

+        then
+          echo "::group::Errors found by git-po-helper"
+          grep -v -e "^level=warning" -e WARNING git-po-helper.out
+          echo "::endgroup::"
+        fi
+        if test "${{ steps.check-commits.outputs.has_warning_msg }}" = "yes"
+        then
+          echo "::group::Warnings found by git-po-helper"
+          grep -v -e "^level=error" -e ERROR git-po-helper.out
+          echo "::endgroup::"
+        fi
+        if test ${{ steps.check-commits.outputs.exit_code }} -ne 0
+        then
+          exit ${{ steps.check-commits.outputs.exit_code }}
+        fi
--
2.33.0