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

[DS-3967] 6x - Migrate Dockerfile to DSpace/DSpace #2134

Merged
merged 8 commits into from Aug 7, 2018

Conversation

@terrywbrady
Copy link
Contributor

commented Jul 25, 2018

@terrywbrady terrywbrady requested a review from pnbecker Jul 31, 2018
@@ -0,0 +1,4 @@
dspace.dir = /dspace

This comment has been minimized.

Copy link
@tdonohue

tdonohue Aug 1, 2018

Member

As noted in today's meeting, let's move this to [src]/src/main/docker/local.cfg

Can we also add some comments at the top of this file to note that this is specific to Docker and used by the DockerFile (or something like that)?

terrywbrady added 5 commits Aug 1, 2018
@ppmdo

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

The local.cfg / build.properties can be gerated at build time as shown here: https://github.com/ppmdo/dspace-docker/blob/93bb794e97b9fa90ed00981721b3de809f1cf5f5/Dockerfile#L52

Although I think that approach can get quite complicated if the dspace.cfg requires many modifications.

Also, It could be useful to build with Mirage2 included and the let the user select the desired theme using an ENV variable as shown here: https://github.com/ppmdo/dspace-docker/blob/93bb794e97b9fa90ed00981721b3de809f1cf5f5/Dockerfile#L63

@terrywbrady

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2018

@ppmdo , thanks for the comments. I had not thought about Mirage2 support.

I am going to test building the images with Mirage2 enabled by default. It will slow the build down a bit, but it should give the end user more flexibility with theme selection. I will share my progress on this.

I envision that the user will use a volume mount in the docker compose file to override any additional properties at runtime (local.cfg, xmlui.xconf). My hope is to minimize the number of variant Docker images that the project will support.

Now that the Dockerfile will exist on each supported branch of DSpace, I have customized the Dockerfiles to provide a minimal local.cfg or build.properties file within each branch. If you take a look at the related PR's you will see that I am providing the files to be overridden.

  • #2137 - 4x (build.properties)
  • #2136 - 5x (build.properties)
  • #2135 - 7x (local.cfg)
@ppmdo

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

@terrywbrady I agree with the files being overridden instead of generated. It's makes it simpler yet flexible.

@terrywbrady

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2018

@ppmdo , I have Mirage2 enabled in the dspace/dspace:dspace-6_x build.

See the following PR to enable the option via docker-compose: DSpace-Labs/DSpace-Docker-Images#51

Note: the build takes a little bit longer to run but the resulting image is roughly the same size.

@ppmdo

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

Tested build and compose on Fedora 27 and CentOS 7. With and without Mirage2, it works flawlessly.

@ppmdo

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2018

Tested in Windows 10 with the following docker-compose file: https://github.com/DSpace-Labs/DSpace-Docker-Images/pull/51/files#diff-e437aab83f63209d609c14191f498e5e

Works without problems, with and without the Mirage2 option.

Copy link
Member

left a comment

👍 I haven't tested, but the Dockerfile & comments within make sense. As @ppmdo has tested this in several scenarios (see above comments), I think this is ready to merge.

@tdonohue tdonohue added the improvement label Aug 7, 2018
@tdonohue tdonohue merged commit b537b68 into DSpace:dspace-6_x Aug 7, 2018
12 of 13 checks passed
12 of 13 checks passed
security/snyk - dspace-spring-rest/pom.xml (DSpace) Failed to detect issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - dspace-api/pom.xml (DSpace) No new issues
Details
security/snyk - dspace-oai/pom.xml (DSpace) No new issues
Details
security/snyk - dspace-rdf/pom.xml (DSpace) No new issues
Details
security/snyk - dspace-rest/pom.xml (DSpace) No new issues
Details
security/snyk - dspace-services/pom.xml (DSpace) No new issues
Details
security/snyk - dspace-solr/pom.xml (DSpace) No new issues
Details
security/snyk - dspace-sword/pom.xml (DSpace) No new issues
Details
security/snyk - dspace-swordv2/pom.xml (DSpace) No new issues
Details
security/snyk - dspace/modules/pom.xml (DSpace) No new issues
Details
security/snyk - dspace/pom.xml (DSpace) No new issues
Details
security/snyk - pom.xml (DSpace) No new issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.