-
Notifications
You must be signed in to change notification settings - Fork 20
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
Fix build issues in CI #248
Conversation
* Feat enable rendering of <img> tags in markdown * Feat add chromium dependencies to Dockerfile
WalkthroughThe recent updates enhance the backend service by integrating headless browser capabilities through the installation of Chromium and its dependencies in the Docker configuration. Additionally, the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- backend/Dockerfile (1 hunks)
- backend/src/files/files.service.ts (1 hunks)
Additional comments not posted (5)
backend/Dockerfile (3)
20-26
: LGTM! The package installations are appropriate.The addition of these packages is necessary for enabling headless browser functionality with Puppeteer.
28-28
: LGTM! The environment variable is correctly set.Setting
PUPPETEER_SKIP_CHROMIUM_DOWNLOAD
totrue
optimizes the build process by avoiding redundant Chromium downloads.
29-29
: LGTM! The environment variable is correctly set.Setting
PUPPETEER_EXECUTABLE_PATH
ensures Puppeteer uses the installed Chromium executable.backend/src/files/files.service.ts (2)
22-22
: LGTM! The type declaration enhances type clarity.Explicitly typing the
markdown
property asMarkdownIt
improves code readability and TypeScript support.
29-33
: LGTM! The initialization is clear and configurable.Initializing
markdown
with specific options enhances configurability and makes the setup more explicit.
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.
Thank you for your contribution.
I have left some comments on formatting the file 😄
* Chore add description for install puppeteer in Dockerfile * Chore add newline in Dockerfile according to POSIX
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- backend/Dockerfile (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- backend/Dockerfile
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.
LGTM. Let's check the docker build will work.
@minai621 Seems like it has failed again: https://github.com/yorkie-team/codepair/actions/runs/10076846631 |
@minai621 Well I think the current Is there any other module to make pdf? Because the latest version of |
I think that we have to change approach to export markdown. |
@minai621 @devleejb Okay I think I have fixed the issue. I thought the image building stage of the Dockerfile was failing, but the build was actually failing in the CI stage of the Dockerfile. I will revert all the undos and enable this feature into the main. Edit: Actually, I think we don't need to CI in the Dockerfile, since the purpose of the Dockerfile is to build container image. I think checking integrity with CI in Dockerfile is out of scope. |
@krapie I mistakenly thought the build tests were being conducted in the CI. Because of this, I believed the CI passed due to the different environment when building the Dockerfile. It seems necessary to test the build in the CI. |
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes