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

Change build strategy to support PDF conversion via LibreOffice #9

Merged
merged 8 commits into from
Dec 21, 2019

Conversation

jujaga
Copy link
Member

@jujaga jujaga commented Dec 20, 2019

Description

This PR focuses on enabling the PDF conversion capability in the Carbone.js library. To this end, the core change involves us switching out from an S2I build to a custom Alpine image with the addition of LibreOffice components to allow PDF conversion to happen.

  • Fix issue with temporary files not cleaning up after completion
  • Simplify temporary file extension handling

We switch off of S2I to an Alpine image in order to have more control over the build process, as well as have a leaner docker image. While S2I is capable of handling standard node applications, the addition of LibreOffice introduces another layer of complexity that S2I honestly wasn't designed to handle.
For reference, the proposed Alpine image is ~700MB in size, whereas an equivalent build extending from the S2I image would be ~2.1GB in size.

[SHOWCASE-405]
[SHOWCASE-455]

Types of changes

New feature (non-breaking change which adds functionality)
Documentation (non-breaking change with enhancements to documentation)

Checklist

  • I have read the CONTRIBUTING doc
  • I have checked that unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the OpenAPI 3.0 v*.api-spec.yaml documentation (if appropriate)
  • I have added necessary documentation (if appropriate)

Further comments

Related Issues: carboneio/carbone#46

Change content type to pdf if pdf

Signed-off-by: Jeremy Ho <[email protected]>
Add in work in progress working Dockerfile
Simplify temp file extension handling

Signed-off-by: Jeremy Ho <[email protected]>
@jujaga jujaga added the enhancement New feature or request label Dec 20, 2019
@jujaga jujaga self-assigned this Dec 20, 2019
@jujaga jujaga force-pushed the feature/libreoffice branch 2 times, most recently from b694071 to 5b9907b Compare December 20, 2019 20:32
@ghost
Copy link

ghost commented Dec 20, 2019

Collapse Dockerfile into app.bc.yaml
Update openshift template indentations

Signed-off-by: Jeremy Ho <[email protected]>
@jujaga jujaga force-pushed the feature/libreoffice branch from 5b9907b to 24182e2 Compare December 20, 2019 21:36
@jujaga jujaga changed the title Feature/libreoffice Change build strategy to support PDF conversion via LibreOffice Dec 20, 2019
@jujaga jujaga marked this pull request as ready for review December 20, 2019 22:04
Add notes on the build config Docker strategy

Signed-off-by: Jeremy Ho <[email protected]>
@jujaga jujaga added the documentation Improvements or additions to documentation label Dec 20, 2019
@ghost
Copy link

ghost commented Dec 20, 2019

Copy link
Contributor

@usingtechnology usingtechnology left a comment

Choose a reason for hiding this comment

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

Couple of minor cleanups on the documentation; just to avoid confusion.

I don't think this works for odt to pdf? Are we not verifying that this dockerfile will kick off the pdf conversion?

app/src/components/docGen.js Show resolved Hide resolved
openshift/README.md Outdated Show resolved Hide resolved
openshift/README.md Outdated Show resolved Hide resolved
openshift/README.md Show resolved Hide resolved
openshift/app.bc.yaml Outdated Show resolved Hide resolved
Drop unused nodejs:10 ImageStream from buildconfig

Signed-off-by: Jeremy Ho <[email protected]>
Copy link
Member Author

@jujaga jujaga left a comment

Choose a reason for hiding this comment

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

DO NOT MERGE with the temporary Line 32 hardcode in the commit history!

@jujaga jujaga force-pushed the feature/libreoffice branch from c9c76f4 to 00b1778 Compare December 21, 2019 00:59
Copy link
Contributor

@usingtechnology usingtechnology left a comment

Choose a reason for hiding this comment

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

👍 nice work getting that onto alpine!

@usingtechnology usingtechnology merged commit 5b22388 into bcgov:master Dec 21, 2019
@jujaga jujaga deleted the feature/libreoffice branch December 21, 2019 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants