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

PRをgit merge --ff-onlyできるようにする #1

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Feb 26, 2023

PRを--ff-onlyの形でマージできる、ラベルで起動するActionを追加します。

動くかどうか試してないのでdraftです。

# https://github.com/robotology/gh-action-nightly-merge/pull/13
git config --global --add safe.directory "$GITHUB_WORKSPACE"

git remote add target "https://x-access-token:${GITHUB_TOKEN}@github.com/$GITHUB_REPOSITORY.git"
Copy link
Member Author

Choose a reason for hiding this comment

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

参考にしたgh-action-nightly-mergeのドキュメントに気になる一言がありました。

https://github.com/robotology/gh-action-nightly-merge#push_token

Useful for pushing on protected branches.

personal tokenを発行すればbranch protectionを貫通できるということ...?

Copy link
Member

Choose a reason for hiding this comment

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

リポジトリの管理者 or チームにBypass branch protection権限がある人はブランチ保護を無視できたはず…?

Copy link
Member Author

Choose a reason for hiding this comment

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

最高の権限があったとしても手元からgit push origin mainしたら怒られたと思うので、それが謎でした。このURLではそうではないということですかね...

@qryxip
Copy link
Member Author

qryxip commented Mar 4, 2023

動いたのでdraftを解除します。
qryxip/actions-playground#1

注意点としては、リポジトリのsettingsから次の設定をオンにする必要があるようです (organization下のリポジトリの場合、多分organization側でも設定が必要)。
image

@qryxip qryxip marked this pull request as ready for review March 4, 2023 14:43
@Hiroshiba
Copy link
Member

fast forwardでマージするの、面白いなと思いました!!!
mainにpushする必要が出てくるのでなかなかむずいし、でも自動半自動化しておきたいしという分野ですねぇ(発想がなかった)

やっぱり権限周りがちょっと危ないかもと思いました!
(それなりに気を配れば大丈夫そうですが)

大対策をいくつか思いつきました!

そもそもgithubにsync fork機能がありそうでした。
(コンフリクト発生時の挙動はちょっと確認してないです)
image

あと、microsoft/mainをマージするためのbufferブランチ(と勝手に読んでるもの)をPRする手もありそうだなと思いました。
VOICEVOXのブランチ戦略のこちら↓で、release-0.1となっているのがmicrosoft/mainなイメージです。
https://github.com/VOICEVOX/voicevox_project/blob/main/docs/%E3%83%96%E3%83%A9%E3%83%B3%E3%83%81%E6%88%A6%E7%95%A5.md#release%E3%83%96%E3%83%A9%E3%83%B3%E3%83%81%E3%82%92main%E3%83%96%E3%83%A9%E3%83%B3%E3%83%81%E3%81%AB%E3%83%9E%E3%83%BC%E3%82%B8%E3%81%99%E3%82%8B%E3%81%A8%E3%81%8D%E3%81%AB%E3%82%B3%E3%83%B3%E3%83%95%E3%83%AA%E3%82%AF%E3%83%88%E3%81%8C%E7%99%BA%E7%94%9F%E3%81%99%E3%82%8B%E5%A0%B4%E5%90%88

@Hiroshiba
Copy link
Member

あーーー でもupstreamのmainをマージしたくないんですよね・・・。
upstream側がリリースを切った(いろいろチェックした)ものをマージするのがトラブル防止になると思っています。

sync forkはタグを選んでマージができないのであれば、難しそうかもです。

@qryxip
Copy link
Member Author

qryxip commented Mar 25, 2023

sync forkですが、コンフリクト解消の機能は全く無さそうでした。あと対象をタグにできたりもできなさそうです。
image

またmicrosoft/onnxruntimeの状況ですが、microsoft#12606以降Rustバインディングが一切更新が無く放置されており、正式なリリースがされるのはまだ先と見てよいと思います。そのためこのリポジトリではしばらくの間、

  • relブランチでタグ(e.g. v.1.14.1)をベースにlibonnxruntimeの改造を行い
  • mainブランチでmainをベースに新生onnxruntime-rsの改造を行う

ということをすることになるのかなと思っています。

@Hiroshiba
Copy link
Member

relブランチでタグ(e.g. v.1.14.1)をベースにlibonnxruntimeの改造を行い
mainブランチでmainをベースに新生onnxruntime-rsの改造を行う

良さそうに思いました!
頂いたこちらのPRはどうしましょう・・・?

@Hiroshiba
Copy link
Member

Hiroshiba commented Mar 28, 2023

個人的には、本家mainを取り込みたいときは、本家のbranchをこちらにプルリク作成するのもありかなと思いました。
取り込む頻度が少なそうで、運用ルールをまとめるほうが大変な気がするなぁと。

でもせっかく作っていただいたので、取り込みたい気持ちもあります。
なので、めちゃくちゃ雑でもいいので起動方法メモをREADMEを書き足すとかどうでしょう。
今のREADMEの上に VOICEVOX onnxruntimeみたいな項目を設けて雑にこのworkflowの使い方を紹介する項目を追加し、その下を---で区切って本家READMEを続ける感じを考えてます。

@qryxip
Copy link
Member Author

qryxip commented Mar 29, 2023

思ったのですが、PRのテンプレートにも以下のようなものを付けた方がいいのかなと。

<!-- ラベル`ff-merge`を付けると、ワークフロー`FF merge`が作動して`--ff-only`のマージが行われる。 --->
- [ ] 本PRは`--ff-only`でマージする。GitHubのUIでマージされるべきではない

(なんかうっかり普通にマージしちゃいそうなんですよね...)

@Hiroshiba
Copy link
Member

たしかに!

@Hiroshiba
Copy link
Member

Hiroshiba commented Apr 16, 2023

READMEじゃなくてworkflowのymlファイルの一番上での説明とかでも良いかもです。
どちらでも!

@qryxip
Copy link
Member Author

qryxip commented Apr 23, 2023

workflow fileにコメントとして書きました。

あとこれもpull request templateに追加しました。
#1 (comment)

@qryxip
Copy link
Member Author

qryxip commented Apr 23, 2023

@y-chan すみません。このPRですがSHAREVOXで同じことができないかと思っています。どうでしょうか?

「upstreamのマージ」を行う機会が多いSHAREVOXで扱いやすいような形が、このVOICEVOX/onnxruntimeにも適しているんじゃないかと思っています。

とりあえずこのPRのアプローチのデメリットとしては

  • branch protectionを切る必要がある

あたりでしょうか。代替としては手元でBashスクリプトを動かすのが挙げられると思います。
(追記) いや手元のBashでも、多分personal tokenは発行しないとbranch protectionは突破できませんね... 微妙かも

@y-chan
Copy link
Member

y-chan commented Apr 23, 2023

すみません...!ちょっと意図が掴めてないのですが、SHAREVOX側でVOICEVOX側の変更をマージする形で、このActionの運用を試してみるって感じですかね...?
私は大丈夫なので、PRだけ作ってもらえると助かるかなと...!(違うのであればごめんなさい)

@qryxip
Copy link
Member Author

qryxip commented Apr 23, 2023

はい、そうです。

このアプローチ自体にbranch protectionを切る必要がある等のデメリットがあるため、SHAREVOXとしてそもそも興味があるか&デメリットが飲めるかの確認がまずしたかったです(すみません、言葉が足りてませんでした…)。

大丈夫なのであればPRを作らせていただきます…!

@qryxip
Copy link
Member Author

qryxip commented Apr 23, 2023

@Hiroshiba すみません、このPRは一旦draftでいいでしょうか...? (このリポジトリでのff-mergeの出番は多分しばらく無いので)

@Hiroshiba
Copy link
Member

もちろん大丈夫です!!

@Hiroshiba
Copy link
Member

課題を思いついたのでメモです。

actions/checkout@v3は通常depth=1でcloneをします。
なのでff-onlyでマージしても、コンフリクトしているかどうかのチェックはできていないかもしれません。
depth=0でcheckoutすると大丈夫ですがonnxruntimeが凄まじい履歴を持っているのでちょっと大変かも。

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.

4 participants