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

Move unnecessary deps from production image into devtest image #54

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

manics
Copy link
Member

@manics manics commented Sep 30, 2020

This is extracted from #38

It moves java/javac into a devtest image, and adds other tools including git.
Note this is Java 8, in keeping with the current OMERO.web standalone image. Now Java 11


E.g. make VERSION=x.y.z docker-build

openmicroscopy/omero-web-devtest              x.y.z                          b19c46a849e4        45 seconds ago      1.29GB
openmicroscopy/omero-web-devtest              x.y.z-0                        b19c46a849e4        45 seconds ago      1.29GB
openmicroscopy/omero-web-standalone           x.y.z                          e1faf515dec9        5 minutes ago       932MB
openmicroscopy/omero-web-standalone           x.y.z-0                        e1faf515dec9        5 minutes ago       932MB
openmicroscopy/omero-web                      x.y.z                          560e614ebb93        5 minutes ago       905MB
openmicroscopy/omero-web                      x.y.z-0                        560e614ebb93        5 minutes ago       905MB

E.g. `make VERSION=x.y.z docker-build`
```
openmicroscopy/omero-web-devtest             x.y.z               b109930c8052        27 seconds ago      1.2GB
openmicroscopy/omero-web-devtest             x.y.z-0             b109930c8052        27 seconds ago      1.2GB
openmicroscopy/omero-web-standalone          x.y.z               d23d308c0d4f        9 minutes ago       869MB
openmicroscopy/omero-web-standalone          x.y.z-0             d23d308c0d4f        9 minutes ago       869MB
openmicroscopy/omero-web                     x.y.z               3331af321dd0        9 minutes ago       862MB
openmicroscopy/omero-web                     x.y.z-0             3331af321dd0        9 minutes ago       862MB
```
@sbesson
Copy link
Member

sbesson commented Sep 30, 2020

So trying to understand the scope of the three images as defined in this PR, my understanding is the following

  • omero-web: minimal OMERO.web image
  • omero-web-standalone: minimal image + OMERO.web apps
  • omero-web-devtest: minimal image + developer tools

Main question is who will be consuming the third one and for what intent? Is there value in having the third image also including the apps?

@snoopycrimecop
Copy link
Member

snoopycrimecop commented Oct 1, 2020

Conflicting PR. Removed from build DOCKER-merge#1279. See the console output for more details.
Possible conflicts:

--conflicts Conflict resolved in build DOCKER-merge#1280. See the console output for more details.

@manics
Copy link
Member Author

manics commented Oct 1, 2020

The devtest image is for use in omero-test-infra. This is the only reason Java is currently installed in the standalone image. With the addition of pre-commit we now also need git so rather than installing more dev dependencies in a production image I think we should put in a separate one.

@joshmoore
Copy link
Member

  • omero-web-devtest: minimal image + developer tools

... : standalone image + developer tools

Copy link
Member

@joshmoore joshmoore left a comment

Choose a reason for hiding this comment

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

I'm not a huge fan of the name devtest but the only thing better I can come up with is dev. I do wonder how ome/omero-web#224 is going to impact this. Will this repo become standalone and dev? If so, should this repo be "standalone" and then another could be "dev"? I think I'm missing the overall plan here, but I'm not against this if it helps us make progress toward whatever that goal is.

wget \
zip

# Shouldn't be necessary any more since OMERO.web/bin/omero is a Python script but included anyway from previous image used in https://github.com/ome/omero-test-infra
Copy link
Member

Choose a reason for hiding this comment

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

What would take to strip it from test-infra?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know, though I suspect it might already work. My intention in this PR was to minimise changes that might impact omero-test-infra, but if you're happy with larger changes I can try removing it.

@manics
Copy link
Member Author

manics commented Oct 1, 2020

ome/omero-web#224 can eventually replace omero-web (minimal OMERO.web image). This repo would then contain the standalone image, and the devtest. You're right that we could split the repos into single image repos for the standalone and dev/devtest images. Decisions to make are:

@joshmoore
Copy link
Member

What's the driver for holding off til 6.0.0? Is there a breaking change?

@manics
Copy link
Member Author

manics commented Nov 11, 2020

IIRC at least one person previously expressed reservations about changing the setup of the Ansible playbook used to build the Dockerfile in case anyone depended on it. I don't hold that view, I think we should treat the documented configuration directories as forming the external contract with downstream images, but document any internal changes. If everyone's agreed with that then once this is reviewed I don't see a blocker to releasing.

Otherwise we can release, but with a non-standard tag to indicate it's a new image.

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build DOCKER-merge#1629. See the console output for more details.
Possible conflicts:

  • Upstream changes
    • requirements.yml

--conflicts

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