Skip to content
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

github_changelog_generator による変更履歴の変更を実装 #1

Merged
merged 38 commits into from
Dec 11, 2018

Conversation

m-tmatma
Copy link
Member

@m-tmatma m-tmatma commented Dec 8, 2018

@m-tmatma
Copy link
Member Author

m-tmatma commented Dec 8, 2018

fork だとビルド通るが、
https://ci.appveyor.com/project/m-tmatma/changelog-sakura/builds/2086367

PR だとビルド通らない。
https://ci.appveyor.com/project/sakuraeditor/changelog-sakura/builds/20863671

ちゃんと設定つもりなのにな~

setting

@berryzplus
Copy link

このPRは何かしたほうが良い?

何気に appveyor 設定見れなかったです。

@m-tmatma
Copy link
Member Author

m-tmatma commented Dec 8, 2018

このPRは何かしたほうが良い?

調査中です。

何気に appveyor 設定見れなかったです。

admin のチームに権限ついてなかったのでつけました。

@k-takata
Copy link
Member

k-takata commented Dec 8, 2018

PR だとビルド通らない。

今回の原因がそれかは見ていませんが、PRではセキュリティー上の理由により一部の機能が制限されています。悪意を持った誰かがPRを作って、情報を抜き取られたり、token等を悪用されると困りますから。

@m-tmatma
Copy link
Member Author

コメントを足しました。

@m-tmatma
Copy link
Member Author

* 定期実行の仕組みはどうなったのか?
  -> 本PRには含まれていなかったので継続案件ですよね。-> #6 を立てました。

そうです。
これには関してはこのプロジェクトでは誰もやったことがないので
appveyor にお願いするところからです。

@m-tmatma
Copy link
Member Author

本PRを見る限り appveyor.yml で完結していると思うので「AppVeyorのUIからは特別な設定はしていない」ですよね?

CHANGELOG_GITHUB_TOKEN の環境変数の設定をしていましたが、
secure 変数の値の生成のために使用したものなので削除しました。

→ 現状では UI で設定しているのはドキュメントで説明している Enable secure variables in Pull Requests from the same repository only のみです。

@m-tmatma
Copy link
Member Author

いずれにせよ、sakura-editor/changelog-sakura に PR を送ること自体が希だと思うので、むしろサクラエディタ以外のプロジェクトで使う人向けの情報になっていると思います。ですよね?

自分も含めたサクラエディタの開発者がドキュメントを読むだけで仕組みを理解できるようにするのと、他のプロジェクトで真似してもらってもいいと思って詳細に書いています。

@m-tmatma
Copy link
Member Author

また、「有効か判定するロジック」の表の正しさは未検証です(試行錯誤しながら検証すると軽く1時間くらいかかると思うのですがそんなに時間を割けないのですみません)

APPVEYOR_REPO_NAME は、appveyor のビルド対象の(アカウント名も含めた)リポジトリ名です。
APPVEYOR_PULL_REQUEST_HEAD_REPO_NAME は PR の送信元の(アカウント名も含めた)リポジトリ名です。

同じリポジトリからの PR であれば両者は一致します。
また PR でなければ APPVEYOR_PULL_REQUEST_HEAD_REPO_NAME は空になります。
ローカルビルドの場合は、両方とも空になります。

PR でなければ、secure 変数が有効なのが期待値なので、changelog の生成を試みます。
同じリポジトリからの PR であれば、Enable secure variables in Pull Requests from the same repository only の設定によって secure 変数が有効なのが期待値なので、この場合も changelog の生成を試みます。

ところが別のリポジトリからの PR であれば、secure 変数は無効なので、changelog の生成を試みずに正常終了します。

@KageShiron
Copy link
Member

問い合わせは雑にTwitterとかで投げたら対応してくれます。まだ誰もやってなければ私が聞いてみましょうか?

http://blog.esora.xyz/appveyor-schedule

@m-tmatma
Copy link
Member Author

問い合わせは雑にTwitterとかで投げたら対応してくれます。
http://blog.esora.xyz/appveyor-schedule

Twitterよりは以下で依頼するのがいいと思っています。
https://github.com/appveyor/ci/issues

過去に依頼実績がありますし、
チケットや PR に自動的にリンクが貼られるので管理しやすいと思います。

私が聞いてみましょうか?

この PR がマージされたらお願いします。

@KageShiron
Copy link
Member

issueがつながるのは良さそうですね。AppVeyorのコラボレータに私を追加しといたほうがいいかもしれません。

@m-tmatma
Copy link
Member Author

AppVeyorのコラボレータに私を追加しといたほうがいいかもしれません。

すでに入ってますよ。

@takke
Copy link
Member

takke commented Dec 10, 2018

自分も含めたサクラエディタの開発者がドキュメントを読むだけで仕組みを理解できるようにするのと、他のプロジェクトで真似してもらってもいいと思って詳細に書いています。

そうですよね。想定読者がこの仕組みを導入したい人、になっているので使いたい人用の説明が必要だな、という観点で指摘させていただきました。

そして、#3 も立てておいて気づかなかったのが恥ずかしいんですが、「他のプロジェクトで真似してもらう」にはこれが何をする仕組みなのかがそもそも README.md に記載されていないことに気づきました。
ちょっと長くなってきたので #7 を立てておきます。

@takke
Copy link
Member

takke commented Dec 10, 2018

また、「有効か判定するロジック」の表の正しさは未検証です(試行錯誤しながら検証すると軽く1時間くらいかかると思うのですがそんなに時間を割けないのですみません)
...
ところが別のリポジトリからの PR であれば、secure 変数は無効なので、changelog の生成を試みずに正常終了します。

ご説明ありがとうございます。その動き自体はバッチファイルを見て読み解けていました。
ただ、例えば APPVEYOR_PULL_REQUEST_HEAD_REPO_NAME が空になる、といったあたりを具体的にそれを追試するのは環境を用意するのが大変なのでパスさせてください。
「sakura-editor/sakuraのChangelogを作る」という目的には本質的に無関係な部分だと思うので問題があれば別PRで対処でいいと思います。

@m-tmatma
Copy link
Member Author

ただ、例えば APPVEYOR_PULL_REQUEST_HEAD_REPO_NAME が空になる、といったあたりを具体的にそれを追試するのは環境を用意するのが大変なのでパスさせてください。

#1 (comment)

に appveyor での実行結果を貼っていて、
APPVEYOR_REPO_NAME および APPVEYOR_PULL_REQUEST_HEAD_REPO_NAME の値のトレースを含む実行結果があるので、説明の通りの動きになっているのかをログ上で確認いただくだけで十分かと思います。

そのときのソースコードも appveyor のリンクをたどれば確認できます。

@m-tmatma
Copy link
Member Author

これが何をする仕組みなのかがそもそも README.md に記載されていないことに気づきました。

コメント足しました。

@takke
Copy link
Member

takke commented Dec 10, 2018

そのときのソースコードも appveyor のリンクをたどれば確認できます。

なるほど、確認しました。OKです。

@m-tmatma
Copy link
Member Author

たびたび細かい部分で申し訳ないんですが、「このリポジトリでは ... プロジェクトです。」という表現がちょっとしっくり来ないです。「このリポジトリは」で意図が伝わるのであればそうして欲しいです。

修正しました。

また以下も変えました。

旧: Issue や Pull Request などの情報を外部ツールを使って
新: Issue や Pull Request などの情報を元に外部ツールを使って

@takke
Copy link
Member

takke commented Dec 11, 2018

修正しました。

ありがとうございます!
私の指摘事項は全て解決したので LGTM です。

Copy link
Member

@takke takke left a comment

Choose a reason for hiding this comment

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

改めて LGTM です。

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.

5 participants