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

On Tue, Aug 24, 2021 at 5:03 AM Johannes Schindelin
[off-list ref] wrote:

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

Yes, this is a solution I also want to try at the very beginning. But
some l10n team leaders may fork their repositories directly from
git/git, and name their repo as "{OWNER}/git".  I want to try another
solution: check existence of file "ci/config/allow-l10n" in branch
"ci-config" using a GitHub API, instead of cloning the ci-config
branch and execute the shell script "ci/config/allow-l10n".

A couple more questions/suggestions are below:

quoted

+  git-po-helper:
+    needs: ci-config
+    if: needs.ci-config.outputs.enabled == 'yes'
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v2
+      with:
+        fetch-depth: '0'

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

If we want do a shallow clone, maybe we can try like this:

1.  Make 'git-po-helper' works with a bare repository, so the initial clone
    can be a shallow bare repository without checkout.
2.  Will get two revisions, base and head revision, when the workflow is
    triggered. (In patch v1, base and head revision are named as
    commit_from and commit_to)
3. Fetch the base revision and head revision using command:
    git fetch origin --depth=100 <base> <head>

We used a fixed depth 100, because we don't know how many commits
these two revisions are diverged.

Will feed the result of "git rev-list <base>..<head>" to
"git-po-helper", if depth 100 is not deep enough, rev-list will fail,
and should try to
run "git rev-list <head> -100".

I think the first version of l10n.yml should use a full clone, and try
to refactor later to use a shallow or partial clone.

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.

Good idea.

quoted

+    - uses: actions/setup-go@v2
+      with:
+        go-version: ">=1.16"
+    - name: Install git-po-helper
+      run: |

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

Will do.

quoted

+      run: |
+        if test "${{ github.event_name }}" = "pull_request"
+        then
+          commit_from=${{ github.event.pull_request.base.sha }}
+          commit_to=${{ github.event.pull_request.head.sha }}
+        else
+          commit_from=${{ github.event.before }}
+          commit_to=${{ github.event.after }}
+          if ! echo $commit_from | grep -q "^00*$"

This would probably read better as:

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

It's better than my version "echo .. | grep".

                        [...]
                        ;;
                esac

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

quoted

+          then
+            if ! git rev-parse "$commit_from^{commit}"
+            then
+              git fetch origin $commit_from

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

v2 renamed $commit_from to $base, and renamed $commit_to to $head.

For a force push, the base commit is missing from the initial clone,
and the missing revision can be only accessed (fetched) using a full
SHA. For a "pull_request_target" event, the "head" revision is also
missing, because "pull_request_target" only the base commit is
checkout.

quoted

+            fi
+          fi
+        fi
+        exit_code=0
+        git-po-helper check-commits --github-action -- \
+          $commit_from..$commit_to >git-po-helper.out 2>&1 ||

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

The option "--github-action" will enable "--no-gpg" and
"--no-gettext-back-compatible" options. To make the workflow
"l10n.yml" stable, I introduced the "--github-action" option. You can
see I introduced another option "--github-action-event=<push |
pull_request_event>", and the boolean option "--github-action" can be
omitted.

quoted

+        exit_code=$?
+        echo "::set-output name=exit_code::$exit_code"
+        has_error_msg=
+        has_warning_msg=
+        if test $exit_code -ne 0
+        then
+          has_error_msg=yes

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.

"env.ERROR_MSG" is an environment variable which contains multiple
lines.  Shell script like `if test ${{ env.ERROR_MSG }} != ""` may
break.

quoted

+          if test "${{ github.event_name }}" = "pull_request"
+          then
+            echo "ERROR_MSG<<EOF" >>$GITHUB_ENV
+            grep -v -e "^level=warning" -e WARNING git-po-helper.out |
+              perl -pe 's/\e\[[0-9;]*m//g' >>$GITHUB_ENV

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

Will replace possible "EOF$" from the output file.

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

Will report errors and warnings all together.

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

"git-po-helper" uses the package "logrus" for logging. The default
format of "logrus” to write log to file is for machines, not user
friendly. The only way is provide an additional option "ForceColor" to
it. So I must clear the color code from the output file, if I want to
create a comment for pull request. But the ansi colors are nice to
display in the report of the action workflow.

quoted

+            echo "EOF" >>$GITHUB_ENV
+          fi
+        fi
+        if grep -q -e "^level=warning" -e WARNING git-po-helper.out
+        then
+          has_warning_msg=yes
+          if test "${{ github.event_name }}" = "pull_request"
+          then
+            echo "WARNING_MSG<<EOF" >>$GITHUB_ENV
+            grep -v -e "^level=error" -e ERROR git-po-helper.out |
+              perl -pe 's/\e\[[0-9;]*m//g' >>$GITHUB_ENV
+            echo "EOF" >>$GITHUB_ENV
+          fi
+        fi
+        echo "::set-output name=has_error_msg::$has_error_msg"
+        echo "::set-output name=has_warning_msg::$has_warning_msg"
+    - name: Report errors in comment for pull request
+      uses: mshick/add-pr-comment@v1
+      if: steps.check-commits.outputs.has_error_msg == 'yes' && github.event_name == 'pull_request'
+      continue-on-error: true
+      with:
+        repo-token: ${{ secrets.GITHUB_TOKEN }}

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

The action "mshick/add-pr-comment@v1" needs this token. See:
https://github.com/mshick/add-pr-comment .

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

You can see this nice post from ecco:
https://github.community/t/github-actions-are-severely-limited-on-prs/18179

For a successful git-l10n workflow, there are no errors, but may have
warnings for possible typos found in a ".po" file.  There may be lots
of false positives in these warnings.  If I hide these warnings in the
log of a workflow, git-l10n contributors may not notice them. So I
need a way to create comments in pull requests.

See some typical code reviews for git-l10n:

* https://github.com/git-l10n/git-po/pull/541
* https://github.com/git-l10n/git-po/pull/555

quoted

+        repo-token-user-login: 'github-actions[bot]'
+        message: |
+          Errors found by git-po-helper in workflow ${{ github.workflow }}:
+          ```
+          ${{ env.ERROR_MSG }}
+          ```
+    - name: Report warnings in comment for pull request
+      uses: mshick/add-pr-comment@v1
+      if: steps.check-commits.outputs.has_warning_msg == 'yes' && github.event_name == 'pull_request'
+      continue-on-error: true
+      with:
+        repo-token: ${{ secrets.GITHUB_TOKEN }}
+        repo-token-user-login: 'github-actions[bot]'
+        message: |
+          Warnings found by git-po-helper in workflow ${{ github.workflow }}:
+          ```
+          ${{ env.WARNING_MSG }}
+          ```
+    - name: Report and quit
+      run: |
+        if test "${{ steps.check-commits.outputs.has_error_msg }}" = "yes"

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.

When users check the log of a workflow, they will only expand the
failed step.  So warnings and errors are reported in one step.

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"

Will try.

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