search

sakura-editor / changelog-sakura

changelog for sakura
MIT License
1 stars 3 forks source link

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

Closed m-tmatma closed 5 years ago

m-tmatma commented 5 years ago

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

@takke さんの以下の成果を参考にさせていだきました。

m-tmatma commented 5 years ago

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 commented 5 years ago

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

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

m-tmatma commented 5 years ago

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

調査中です。

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

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

k-takata commented 5 years ago

PR だとビルド通らない。

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

m-tmatma commented 5 years ago

原因および対策判明しました。

原因

まず、 @k-takata さんのコメントの通り、secure 変数は PR の中では デフォルトで 有効になりません。 それはセキュリティ上の理由からです。

参考: https://github.com/appveyor/ci/issues/2355#issuecomment-390803634

対策

appveyor 上の UI の設定で Enable secure variables in Pull Requests from the same repository を 有効にすることによって、同じリポジトリ内の branchから pull request が送られた場合のみ secure 変数の 使用を有効にすることができます。

secure 変数が有効/無効の条件

条件 secure 変数 処理実行の可否
ローカルビルド - ○
ブランチの appveyor ビルドの場合 (Fork) ○ (UI で設定した場合) ○
ブランチの appveyor ビルドの場合 (sakura-editor) ○ ○
PR (送信元: Fork のブランチ) × ×
PR (送信元: sakura-editor のブランチ) ○ ○

PR #1 内の README.md に詳細書いています。

動作確認結果

パターン1

ローカルで実施

→ OK

パターン2 : ブランチの appveyor ビルドの場合 (Fork)

https://ci.appveyor.com/project/m-tmatma/changelog-sakura/builds/20873098

→ OK

パターン3 : ブランチの appveyor ビルドの場合 (sakura-editor)

https://ci.appveyor.com/project/sakuraeditor/changelog-sakura/builds/20873277

→ OK

パターン4 : PR #1 (送信元: Fork のブランチ)

https://ci.appveyor.com/project/sakuraeditor/changelog-sakura/builds/20873100

→ OK (CHANGELOG.md の生成はスキップ)

パターン5 : PR #2 (送信元: sakura-editor のブランチ)

https://ci.appveyor.com/project/sakuraeditor/changelog-sakura/builds/20873359

→ OK

メモ

https://ci.appveyor.com/project/sakuraeditor/changelog-sakura/builds/20873325 は PR #2 (送信元: sakura-editor のブランチ)のパターンだが Enable secure variables in Pull Requests from the same repository only を有効にした後 保存しない状態で実施していた。

m-tmatma commented 5 years ago

レビュー、プリーズ

berryzplus commented 5 years ago

レビュー、プリーズ

目的がよく分っていません。 やってることも把握できてるかどうか怪しいです。

変更履歴を自動生成する仕組みをsakura-editorで動作できるようにする という作業をしてる雰囲気を感じていますが、根拠がありません。

まず、目的を共有してください。 コードのレビューなのか、手順（環境整備）のレビューなのか、期待結果のレビューなのか。

m-tmatma commented 5 years ago

コード、ドキュメント、結果全てです。

m-tmatma commented 5 years ago

変更履歴を自動生成する仕組みをsakura-editorで動作できるようにする という作業をしてる雰囲気を感じていますが、根拠がありません。

https://github.com/sakura-editor/management-forum/issues/53 からの中で何をしようとしているか自明だと思いますが、 何がわからないのか、よくわからないです。

m-tmatma commented 5 years ago

@takke さん

この件に関して一番詳しいと思うのでレビューお願いできますか?

takke commented 5 years ago

わかりました。レビューさせていただきます。 週末はちょっと忙しく、明日以降になると思います。

berryzplus commented 5 years ago

変更履歴を自動生成する仕組みをsakura-editorで動作できるようにする という作業をしてる雰囲気を感じていますが、根拠がありません。

sakura-editor/management-forum#53 からの中で何をしようとしているか自明だと思いますが、 何がわからないのか、よくわからないです。

まったく分かりません。 把握すべきことは @m-tmatma さんがこのやり取りから何を読み取り、 どのように sakura-editor に反映しようとしているかで、 レビューすべきはその思考過程が妥当であるか、明らかな見落としなどの瑕疵がないか、だと思っています。

やり取りの中から必要な情報を読み取れないということは、 ぼくにはレビュアー資格がないということを意味します。 レビューできそうな他の人にお願いしてみてください。

メンバーの了解が必要なようであれば追認はしますので。

berryzplus commented 5 years ago

@takke さん、すみませんお願いします :smile:

m-tmatma commented 5 years ago

  • エンドユーザーが CHANGELOG.md を確認したいとき、どうすればいいのか？ 本PRの結果としては「https://ci.appveyor.com/project/sakuraeditor/changelog-sakura/build/artifacts の CHANGELOG.md をダウンロードしてください」になるかと思います。せめてその1文を README.md の冒頭に追加して欲しいです。

  • しかし、CHANGELOG.md をダウンロードしてローカルで見るのはしんどいです。-> #5 を立てました。

コメントを足しました。

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

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

m-tmatma commented 5 years ago

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

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

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

m-tmatma commented 5 years ago

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

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

m-tmatma commented 5 years ago

また、「有効か判定するロジック」の表の正しさは未検証です(試行錯誤しながら検証すると軽く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 commented 5 years ago

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

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

m-tmatma commented 5 years ago

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

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

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

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

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

KageShiron commented 5 years ago

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

m-tmatma commented 5 years ago

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

すでに入ってますよ。

takke commented 5 years ago

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

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

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

takke commented 5 years ago

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

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

m-tmatma commented 5 years ago

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

https://github.com/sakura-editor/changelog-sakura/pull/1#issuecomment-445505142

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

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

m-tmatma commented 5 years ago

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

コメント足しました。

takke commented 5 years ago

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

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

m-tmatma commented 5 years ago

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

修正しました。

また以下も変えました。

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

takke commented 5 years ago

修正しました。

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