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

MNT-20337/REPO-4534/ATS-484 LibreOffice 6.1.6 #78

Merged
merged 5 commits into from
Aug 20, 2019

Conversation

alandavis
Copy link
Contributor

Can I have your 3 story points for ATS-484 :) Needed to work out if 6.1.6 was compatible with alfresco-jodconverter-core-3.0.1.1 which is used by both the Libreoffice T-Engine and the repo.

README.md Outdated
@@ -16,7 +16,7 @@ Contains the common transformer (T-Engine) code, plus a few actual implementatio

The project can be built by running the Maven command:
```bash
mvn clean install -Plocal
mvn clean install -DskipTests -Plocal
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Requires ActiveMQ to be running and configured at the moment, so you might like to work out what needs to change in the README.md, before closing ATS-484.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we should update the README steps (as needed) so that we do not skip tests when building locally. It might be as a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've updated the line with the proper profiles.

@@ -18,8 +18,8 @@ COPY target/alfresco-docker-libreoffice-${env.project_version}.jar libreoffice-d

RUN ln /alfresco-docker-libreoffice-${env.project_version}.jar /usr/bin/alfresco-docker-libreoffice.jar && \
yum install -y cairo cups-libs libSM && \
test -f libreoffice-dist-5.4.6-linux.gz && \
ln -s libreoffice-dist-5.4.6-linux.gz libreoffice-dist-linux.gz || \
test -f libreoffice-dist-6.1.6-linux.gz && \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I owned this code I would not have the version number in the code. Currently it needs to be updated in 3 places in this file and also in the executor (if the minor version changes).

Copy link
Contributor

Choose a reason for hiding this comment

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

Done. I've added Dockerfile arguments for the LibreOffice & ImageMagick versions.

@montgolfiere
Copy link
Contributor

Yes, of course - all tested contributions are gratefully accepted :-)

Do we know if the unit tests (in this project &/or Repo are sufficient to catch blatant regressions ?

Copy link
Contributor

@eknidev eknidev left a comment

Choose a reason for hiding this comment

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

LGTM

@CezarLeahu
Copy link
Contributor

CezarLeahu commented Aug 19, 2019

I've merged the master branch and also addressed the PR comments.
However, the build will fail at the WhiteSource scan due the the quartz dependency (CVE-2019-13990).

We'll most likely merge this PR once we run a few minor sanity tests.

@CezarLeahu CezarLeahu merged commit 18973d9 into master Aug 20, 2019
@CezarLeahu CezarLeahu deleted the fix/mnt-20337_LO_6.1.6 branch August 20, 2019 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants