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.

Sign up for GitHub

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

Jump to bottom

Feat/135772 155547 pdf converter tsed devcontainer #9280

Merged
merged 17 commits into from
Nov 11, 2024
Merged

Feat/135772 155547 pdf converter tsed devcontainer #9280

merged 17 commits into from
Nov 11, 2024

Conversation

arafubeatbox
Copy link
Contributor

@arafubeatbox arafubeatbox commented Oct 20, 2024 •
edited
Loading

実装内容

pdf-conveter を devcontainer 内で起動できるようにする

参考: https://code.visualstudio.com/remote/advancedcontainers/connect-multiple-containers

  • growi レポジトリを devcontainer で起動する際に、"GROWI-Dev" と "GROWI-PDF-Converter-Dev" を選択できるように変更
    • docker-compose 内の "node" を "app" に変更 (pdf-converter も node アプリのため)
    • app の devcontainer.json を .devcontainer/app に移動
    • .devcontainer/pdf-converter に pdf-converter 用の devcontainer.json を用意
  • apps/app と apps/pdf-converter 両方を起動したい時は、二つの vscode window を起動する

pdf-converter のアプリを turbo 経由で workspace root dir から起動できるようにする

  • task: dev:pdf-converter
    • "dev" は apps/app のために設定されているため、別の task を用意

task

https://redmine.weseek.co.jp/issues/155547

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Oct 20, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: 937a57b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@arafubeatbox arafubeatbox mentioned this pull request Oct 20, 2024
Merged
@arafubeatbox arafubeatbox deleted the branch feat/135772-pdf-page-bulk-export October 20, 2024 09:37
@arafubeatbox arafubeatbox reopened this Oct 20, 2024
Base automatically changed from feat/135772-153347-pdf-converter-app-tsed to feat/135772-pdf-page-bulk-export October 24, 2024 07:27
@arafubeatbox arafubeatbox marked this pull request as ready for review October 25, 2024 02:47
yuki-takei
yuki-takei requested changes Oct 25, 2024
View reviewed changes
.devcontainer/app/devcontainer.json Outdated
Copy link
Member

Choose a reason for hiding this comment

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

pnpm 化に乗じて devcontainer 関連ファイルもかなり変えたので、新しい方式に則ってください

.devcontainer/docker-compose.yml Outdated
volumes:
- ..:/workspace/growi:delegated
- node_modules:/workspace/growi/node_modules
- node_modules_pdf_converter:/workspace/growi/apps/pdf-converter/node_modules
Copy link
Member

Choose a reason for hiding this comment

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

node_modules_pdf_converter volume も不要になると思う
(ただの symlink 置き場なので volume 管理しなくてよい)

turbo.json Outdated
"dev:pdf-converter": {
"cache": false,
"persistent": true
},
Copy link
Member

Choose a reason for hiding this comment

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

リポジトリ内の別のパッケージのビルドに依存するとかがなければ、トップレベルの turbo.json には書かなくてもいい気はする

  • 単に apps/pdf-converter に移動してから pnpm 叩くでもよい
  • トップレベルのディレクトリから実行したいならトップの package.json に追加する( slackbot-proxy:* script がそのような設計)
  • pdf-converter 内の複数スクリプトを叩かせたいなら pdf-converter/turbo.json を用意する

Copy link
Contributor Author

Choose a reason for hiding this comment

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

単に apps/pdf-converter に移動してから pnpm 叩くでもよい

これにしようと思います。apps/pdf-converter でのコマンドも単純な dev にしました。

arafubeatbox
arafubeatbox commented Nov 9, 2024
View reviewed changes
.devcontainer/compose.yml
@@ -40,7 +41,18 @@ services:
- /usr/share/elasticsearch/data
- ../../growi-docker-compose/elasticsearch/v8/config/elasticsearch.yml:/usr/share/elasticsearch/config/elasticsearch.yml

pdf-converter:
# enabling devcontainer 'features' was not working for secondary devcontainer (https://github.com/devcontainers/features/issues/1175)
image: mcr.microsoft.com/vscode/devcontainers/javascript-node:0-20
Copy link
Contributor Author

Choose a reason for hiding this comment

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

docker-compose によって devcontainer が二つ立ち上がりますが、二つ目に立ち上がった devcontainer ではどうしても features が有効になりませんでした。仕様なのかバグなのかは分かりませんが、ドキュメントを漁っても原因がわからなかったため一旦 base image を node が入ったものにして、issue 起票 しました。

arafubeatbox
arafubeatbox commented Nov 10, 2024
View reviewed changes
.devcontainer/app/devcontainer.json
@@ -37,6 +37,9 @@
"vitest.explorer",
"ms-playwright.playwright"
],
"settings": {
"terminal.integrated.defaultProfile.linux": "bash"
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

自分の環境だと zsh でターミナルが起動されてしまい、postCreateCommand で bashrc に書き込んだ設定が読み込まれませんでした。

@yuki-takei yuki-takei merged commit 8420213 into feat/135772-pdf-page-bulk-export Nov 11, 2024
2 checks passed
@yuki-takei yuki-takei deleted the feat/135772-155547-pdf-converter-tsed-devcontainer branch November 11, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuki-takei yuki-takei yuki-takei requested changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@arafubeatbox @yuki-takei