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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Mon, 23 Aug 2021, Jiang Xin wrote:

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> 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:

> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  .github/workflows/l10n.yml | 143 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 143 insertions(+)
>  create mode 100644 .github/workflows/l10n.yml
>
> diff --git a/.github/workflows/l10n.yml b/.github/workflows/l10n.yml
> new file mode 100644
> index 0000000000..60f162c121
> --- /dev/null
> +++ b/.github/workflows/l10n.yml
> @@ -0,0 +1,143 @@
> +name: git-l10n
> +
> +on: [push, pull_request]
> +
> +jobs:
> +  ci-config:
> +    runs-on: ubuntu-latest
> +    outputs:
> +      enabled: ${{ steps.check-l10n.outputs.enabled }}
> +    steps:
> +      - name: try to clone ci-config branch
> +        run: |
> +          git -c protocol.version=2 clone \
> +            --no-tags \
> +            --single-branch \
> +            -b ci-config \
> +            --depth 1 \
> +            --no-checkout \
> +            --filter=blob:none \
> +            https://github.com/${{ github.repository }} \
> +            config-repo &&
> +          cd config-repo &&
> +          git checkout HEAD -- ci/config || : ignore
> +      - id: check-l10n
> +        name: check whether CI is enabled for l10n
> +        run: |
> +          enabled=no
> +          if test -x config-repo/ci/config/allow-l10n &&
> +             config-repo/ci/config/allow-l10n '${{ github.ref }}'
> +          then
> +            enabled=yes
> +          fi
> +          echo "::set-output name=enabled::$enabled"
> +
> +  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).

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.

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

> +        go install github.com/git-l10n/git-po-helper@main
> +    - name: Install other dependencies
> +      run: |
> +        sudo apt-get update -q &&
> +        sudo apt-get install -q -y gettext
> +    - id: check-commits
> +      name: Run git-po-helper
> +      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
		*)
			[...]
			;;
		esac

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

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

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

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

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

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

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

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

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

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