-
Notifications
You must be signed in to change notification settings - Fork 4
add: pull request page preview #1
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
base: main
Are you sure you want to change the base?
Conversation
|
年始早々失礼します 改善点等あれば教えてください |
|
年始早々お疲れ様です. |
|
年始早々速いご返信ありがとうございます このprは急用ではありませんのでごゆっくり確認いただければ幸いです |
hinshiba
left a comment
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.
PRありがとうございます.
いくつか疑問点がありますので返答いただけますと幸いです.
各レビューコメントで書いたこととは別に,PRに応じてPreviewが作成されると,
悪意のあるPRが作成された際にそのコンテンツのPreviewが公開される危険性があるように思えるのですがどうでしょうか?
| @@ -0,0 +1,34 @@ | |||
| name: Build | |||
|
|
|||
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.
| mdbook-version: "latest" | ||
|
|
||
| - name: Build mdbook | ||
| run: mdbook build --dest-dir out |
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.
shellプロパティは必須です.
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.
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.
run が設定されている場合は必須です。
と書いてあるように見えます
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.
すみません...見落としてました。
(なくても動きはするのだが、一応規格上は必須になっているのか...)
| ${{ runner.os }}-cargo- | ||
| - name: Install mdbook-utils | ||
| if: steps.cargo-cache.outputs.cache-hit != 'true' |
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.
同様にshellプロパティの不足
| run: cargo install mdbook-utils | ||
|
|
||
| - name: Generate sitemap | ||
| run: mdbook-utils sitemap -o out/sitemap.xml -b https://rsdocsjp.org/ -y |
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.
同様にshellプロパティの不足
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Checkout |
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.
一貫性のためにpreview.ymlと同様にCheckout gh-pagesにしてほしいです.
| - name: Checkout | |
| - name: Checkout gh-pages |
| git config --global user.name "GitHub Actions" | ||
| git config --global user.email "action@github.com" | ||
| mkdir -p out/preview/${{ env.PAGE_DIR }} | ||
| mv tmp/out /out/preview/${{ env.PAGE_DIR }} |
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.
これは絶対パスではまずいように思えます.
| issue_number: context.issue.number, | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| body: `Preview deployed to: https://${context.repo.owner}.github.io/preview/${process.env.PAGE_DIR}` |
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.
カスタムドメインがあるので
| body: `Preview deployed to: https://${context.repo.owner}.github.io/preview/${process.env.PAGE_DIR}` | |
| body: `Preview deployed to: https://rsdocsjp.org/preview/${process.env.PAGE_DIR}` |
が正しいと思われます.
|
年末年始バタバタしながら作ったのもありまして一部のworkflowに不足があり申し訳ないです。至急修正します。 悪意のあるプルリクエストに関する話といたしましては、.github/workflowsに修正が入っているprに関しては実行において運営側の許可が必要になります。(このprもその都合で実行されていません) そのためコマンドを書き換えられて悪意のあるページを公開されることはありません。 |
こちらの話をしています. |
|
そもそもの話、Environment使えば誰かの承認がいるようにできるので、それで良いのでは? |
承認するまで走らないようにすればいいのでは という話を今しているのですが......
mdをざっと見て問題ないかを確認したらpreview生成しようという話です. |
|
アーカイブ問題に関してはrobots.txtなどを明記し、それでも/preview/*をアーカイブする様でしたら直接問い合わせて削除してもらうのでいいと思うのですがそれではダメですか? それとpreview作成においてこちらが一度見てから実行をかけるとなるとものによってはworkflowで作るのが相当面倒なのですが具体的にどのようなものを考えていますか? |
これは強制力がないので実質的には意味があまりないと考えています.
Environment Protection RulesでApproveするまで生成されないようにするという手を, まあ利便性とセキュリティ/コンプライアンスはトレードオフなので, previewは改行位置やスタイルの評価に便利ですが, @KaiTomotake sanはどう考えていますか? |
|
このあたりは私の専門外ですので、技術的な是非までは判断できません。 |
|
@KaiTomotake san この機能はPRに対してpreviewを作成しますが, |
|
previewはApproveされると公開されるという認識で正しいですか? |
いいえ.これは
なので,どちらの仕様にするかという話です. その"一度見てから実行をかける" を実現する手法が
という案です.(当然面倒です) |
|
私個人の意見としましては、ある程度のリスクはしょうがないと考えています。 |
プレビュー案自体には私も賛成なのですが, そういう意味で言っているのでしたら申し訳ないです. |
私もそう思います |
|
返信遅くなり申し訳ありません。 また他サービスにデプロイする方式やcloudflareのルール設定によってrsdocsjp.orgからはアクセス不可にしrust-developers-jp.github.ioからのみプレビューを閲覧できるようにするという案もありますがどうしますか? |
いえいえ.こちらこそ貢献しようとして頂いてありがとうございます.
機能が満たせるならどちらでも構いませんが,workflow_dispatchのほうが直感的かなという気もします. |
ページプレビューが出るように.github/workflows/preview.ymlを追加しました