-
Notifications
You must be signed in to change notification settings - Fork 24
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
"master ブランチに直接 push しようとしたら中断させる仕組み" を導入するスクリプトを作りました #1282
base: master
Are you sure you want to change the base?
Conversation
おおー、使ってくれてありがとうございます! 以下ちょっと気になった点です。 1. 1.と2.はちょっと対応してみました。めちゃ長くなったけど… |
すっかり反応が遅くなってしまって、申し訳ありません。インラインで書けるところはそのほうがわかりやすそうなので、そちらに回答させていただきますね 🙇 |
# TODO 以下はダミーなので、差し替える https://raw.githubusercontent.com/yochiyochirb/meetups/master/scripts/update_pre_push/pre-push.sample | ||
SCRIPT_URI = 'https://raw.githubusercontent.com/yochiyochirb/meetups/master/members/yucao24hours.md' | ||
|
||
File.open('.git/hooks/pre-push', 'a') do |f| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update_pre_push.rb を実行するときのカレントディレクトリの制限がきつい (repo の root 以外だとこける) ので、git rev-parse --git-dirで取得するなど、meetup リポジトリ内ならOKくらいには緩くしたい
これについては、個人的にはそこまで気にしなくていいかなあと思っていました。
ご参加者のかたにこのスクリプトを実行していただくのはおそらく原則として初参加のミートアップのときだけだと思うのですが、その際にわたしなり隣に座った方なりが「 cd meetups
って打ってからやってくださいね〜」とご案内すればそれで 🆗 だと思っており、それがそこまでの負担にはならないと判断したからです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File.open('.git/hooks/pre-push', 'a') これはモードが a なのは意図通りですか?既存ファイルを消して上書きするのを避けたい?(そうであっても、append されると結局ファイルの内容おかしくなるので、普通に上書きして、上書きされることを警告するので良い気がします)
a
にしたのはまさにおっしゃるとおりで、
既存ファイルを消して上書きするのを避けたい
と思ったからです。そのユーザが既に何かしらの pre-push hook を設定していたとしたら、知らない間にそれがなくなってしまうことになるので... 😱
でも、たしかに2つ以上 hook script が書かれてしまうと内容が変になってしまいますね 😥
理想としては、上書きする前に警告メッセージが出るとかで、ユーザの承認操作なしには上書きされないようにできたらいいなあと思っていました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
その際にわたしなり隣に座った方なりが「 cd meetups って打ってからやってくださいね〜」とご案内すればそれで 🆗 だと思っており、それがそこまでの負担にはならないと判断したからです。
そうですね。でも個人的な心情として、プログラムをちょっと直せば解決できそうなことは運用面での配慮に回したくないという気持ちがあります…
書いた通り、update スクリプトファイルの実体のパスを基準に git rev-parse --git-dir
を使えば確実に .git
ディレクトリが (meetups の外であろうが) 取得できるのでそうしたいなあと思います。
その際に git
コマンドをスクリプトから呼び出すので、git
コマンドに対してパスが通っていることだけは前提として必要になりますが、むしろ活動の中ではコマンドラインから Git を使うことをデフォルトのやり方として進めていて、以降で Git 操作で詰まった場合のサポートなども CLI ベースで伝えることがほとんどなので、パスが通っていることを最初の時点で確認できるのはむしろ良いことかなと思います。
理想としては、上書きする前に警告メッセージが出るとかで、ユーザの承認操作なしには上書きされないようにできたらいいなあと思っていました。
それがいいですね!
確認ありがとうございます! インラインでコメント追加しましたけど、コメントしてくれたところも実はほぼ対応したやつがあって、せっかく作ったのであとで別で PR 送らせてもらいますので、直接その内容を確認してもらった方が良さそうです。よろしくお願いします。 |
やったこと
ユーザが master ブランチに直接 push できないようにする制御処理を、ユーザ自身のローカル環境に導入するためのスクリプトを作りました。
meetups/ ディレクトリで
を実行するだけで 🆗 なようになっています(なります)。
背景
この meetups リポジトリでは原則として、master ブランチへ直接 push するような運用はしていただかないようにお願いしています。
なぜなら、master ブランチにあるファイルはわたしたち全員の共有すべき持ち物であり資産ですので、直接いじらずにまずはトピックブランチで作業をし Pull Request によるレビューを経て、キレイな状態にしてからマージしていきたいからです。
そのため、万が一 master に向けて push をしようとしてしまったときに、それを未然に防ぐための仕組みを取り入れたいと思い、今回の PR を作成しました。
作成したファイルについて
💡
scripts/update_pre_push/pre-push.sample
には、Git でgit push
をした際に呼ばれる pre-push hook と呼ばれる仕組みを使って、ユーザが master ブランチに向けて push しようとしている場合はエラーメッセージを出力して push を中断させるという処理が記述されています。こちらのスクリプトは @5t111111 さんが作ってくださいました 🎊 🙇(
[ALLOW_MASTER]
という文字列からはじまるコミットメッセージのコミットを含む場合のみ、中断させずに push ができます。ただしこちらはコミュニティ運営用として使うことを前提にしているため、通常の操作では使わない運用とします。)💡
scripts/update_pre_push/update_pre_push.rb
を実行すると、上記のスクリプト内容をユーザ自身のローカル環境にある .git/hooks/pre-push に追記し、実行可能な状態にします。💡ユーザのみなさんが使っていらっしゃる PC の環境に関係なく実行できる状態にしたかったので、シェルスクリプト実行ではなくあえて Ruby によるスクリプトを作成しました。
対応ストーリー
connects to #1203