Skip to content

GitHub Actions #37

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

GitHub Actions #37

wants to merge 15 commits into from

Conversation

sfujiwara
Copy link
Member

#36 何か CI 的なのがあった方が気分が盛り上がるかなと思って GitHub Actions で ./bin/run-docker コマンドがちゃんと正しく動くかチェックするようにしてみた

@sfujiwara sfujiwara requested a review from chie8842 November 22, 2019 17:55
@sfujiwara sfujiwara self-assigned this Nov 22, 2019
runs-on: ubuntu-18.04

steps:
- uses: actions/checkout@v1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

これだとactions/checkout@v1 Dockerコンテナの上でさらにコンテナを起動することになるので、
uses: docker://tfug/proofreading:latestとして run:の中をbin/run tensorflow/docs master result.txtとしたほうが自然な気がします。

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

僕もまだ GitHub Actions あんまりちゃんとわかっていないけれど、

  • uses: actions/checkout@v1 みたいな use は処理の単位の区切り (action と呼ぶらしい) で、その単位ごとに任意のイメージを使ったり予め用意した処理を呼び出すことができる
    • Action を指していたり Docker image そのものを指していたりしてややこしい (?)
    • actions/checkout は公式に用意されているやつで CircleCI で言う checkout と同じ
    • なので後ろの RedPen による check とかは actions/checkout とは別のコンテナで動かす形になっている
      • イメージを指定しなければ ubuntu-latest 上になる

みたいな感じっぽいです。

YAML がわかり辛いなと思ったので、こんな感じで修正してみました:

name: actions

on: [push]

jobs:
  build:

    runs-on: ubuntu-latest

    steps:

      - name: Checkout
        uses: actions/checkout@v2

      - name: Check diff
        uses: docker://tfug/proofreading:latest
        with:
          args: ./bin/run tensorflow/docs master
    ...

@@ -1,5 +1,7 @@
# Proofreading for TensorFlow docs translation

![](https://github.com/tfug/proofreading/workflows/actions/badge.svg)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

このCIジョブはすべてのブランチでのpushイベントに対して発火するように設定されていますが、その結果をバッチに出すとmasterはちゃんと動作しているのにfailが表示されてしまうということになり、メンテナンスされていないかのように受け取られます。

このリポジトリ上でGithub Actionsを使って実現できそうなことは以下の3つがあります。(他にもブランチの定期削除とかもできますが)

  1. PRが出ているブランチに対するテスト
  2. mergeされたmasterブランチの内容に対するテスト
  3. on.scheduleを使って定期的にtensorflow/docsのmasterの内容をチェックする

このPRでの内容は上記の 1.と思ってレビューしましたが、バッチを出すとしたら2.3.の結果に対して出すのが良いかと思います。

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

たぶん今の状態だとバッジ上のステータスはブランチに関わらずデフォルトブランチである master のステータスが表示されるので、懸念しているような master 以外のブランチで CI 失敗して failed が表示されるということにはならないかと。
https://help.github.com/ja/actions/automating-your-workflow-with-github-actions/configuring-a-workflow#adding-a-workflow-status-badge-to-your-repository

@@ -1,2 +1,3 @@
# Temporary directory to clone GitHub repository
ghrepos
result.txt
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bin/runでは結果のファイルはresult.txtでなくても第3引数で指定できるようになっています。
#33 (comment) でコメントしましたが、result.txtに固定して引数で渡さないようにするのがいいかなと思っています。

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここは #42 で修正済み
master 側で大きい修正が入ったので、いったん master をこっちの branch にマージして、ついでに conflict の解消とかしました

@sfujiwara sfujiwara mentioned this pull request Dec 17, 2019
@sfujiwara
Copy link
Member Author

これ RedPen で修正の要求が出ると終了コードが 1 になって CI コケた扱いになってしまうのか
どうしようかな...?

@sfujiwara
Copy link
Member Author

./bin/run tensorflow/docs-l10n master all 10 みたいな感じで error limit を optional で設定できるようにして、CI 上では error limit を 10 に緩めてみた

@sfujiwara
Copy link
Member Author

あれ?なんか [2020-02-02 21:08:26.665][ERROR] cc.redpen.Main - The number of errors "35" is larger than specified (limit is "10"). とかあるのに CI 通っちゃってるのでだめっぽいな・・・?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants