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

[DO NOT MERGE] Revert "Do not use VOLUME command in Dockerfiles" #224

Closed
wants to merge 1 commit into from

Conversation

hhorak
Copy link
Member

@hhorak hhorak commented Apr 23, 2018

This reverts commit 987e5c5, because it breaks things: openshift/origin#19470

@hhorak
Copy link
Member Author

hhorak commented Apr 23, 2018

[test-openshift]

@hhorak
Copy link
Member Author

hhorak commented Apr 23, 2018

This effectively reverts #223, but @mfojtik said we should wait for @bparees before merging. Ben, let us know :)

@bparees
Copy link
Collaborator

bparees commented Apr 23, 2018

sclorg/httpd-container#30 hasn't been resolved, so why would we do this?

@bparees
Copy link
Collaborator

bparees commented Apr 23, 2018

since origin was fixed by openshift/origin#19470, i would say nothing further needs to be done.

(other than perhaps introducing a new volume test in origin)

@hhorak
Copy link
Member Author

hhorak commented Apr 23, 2018

sclorg/httpd-container#30 hasn't been resolved, so why would we do this?

It actually was fixed already, just github issues was not closed (I've done it now).

@pkubatrh
Copy link
Member

I thought we wanted to keep the issue open in order to track it for a future fix when Openshift Online is able to work with VOLUMEs

@hhorak
Copy link
Member Author

hhorak commented Apr 23, 2018

@pkubatrh ah, forgot about that, re-opening.

@hhorak
Copy link
Member Author

hhorak commented Apr 23, 2018

@bparees, Now I'm puzzled a bit, from what @mfojtik said I had a feeling that VOLUME set for databases images is actually beneficial, because users can be advised to use PV.. is that not a fair assumption? it seems to me like it's a trade-off between this advice and making s2i working in OpenShift online.. and I'm kinda clue-less what is more important use case. :)

@bparees
Copy link
Collaborator

bparees commented Apr 23, 2018

@hhorak yes that's pretty much the trade off. there's just no good answer today.

but ultimately i'd say making s2i work in online is more important because there is no workaround for that issue. Users not understanding which volume they need to map is unfortunate, but that's in part why we have templates that help set all that up.

@bparees
Copy link
Collaborator

bparees commented Apr 23, 2018

(docs for the images that tell users what the volumes should be can also help)

@hhorak hhorak changed the title Revert "Do not use VOLUME command in Dockerfiles" [DO NOT MERGE] Revert "Do not use VOLUME command in Dockerfiles" Apr 23, 2018
@hhorak
Copy link
Member Author

hhorak commented Apr 23, 2018

@bparees Ok, thanks for you PoV. We did the change deliberately just in one database image at this point, to see what issues we can cause (we hoped for none, but were proofed wrong). I'd wait for further feedback (~month) before doing the same thing again for other images, so we don't break everything at once.
@mfojtik, if there are no other issues and we'll decide removing the VOLUME in other database images, can we expect the same thing fail again on your site?

@bparees
Copy link
Collaborator

bparees commented Apr 23, 2018

@mfojtik, if there are no other issues and we'll decide removing the VOLUME in other database images, can we expect the same thing fail again on your site?

we'd have to scan our tests to see if we have other tests consuming the images you're changing and that expect to see volumes. I wouldn't expect a bunch, we probably just picked the mysql image to exercise new-app.

@hhorak
Copy link
Member Author

hhorak commented Jun 15, 2018

It seems like we're ok to keep the image as it is, without VOLUME. So, closing.

@hhorak hhorak closed this Jun 15, 2018
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.

3 participants