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

Alpine openjdk #64

Merged
merged 10 commits into from
Mar 16, 2017
Merged

Alpine openjdk #64

merged 10 commits into from
Mar 16, 2017

Conversation

rbellamy
Copy link
Contributor

@rbellamy rbellamy commented Oct 3, 2016

Per #62

The two commits should be tagged alpine-openjdk-v7.91.2 and alpine-openjdk-v8.92.14 and their docker images tagged per the VERSIONS.md file.

G. Richard Bellamy added 2 commits October 2, 2016 17:27
* alpine-openjdk - informed by the official alpine:7-jdk image found on docker.io.
  https://github.com/docker-library/openjdk/blob/master/7-jdk/alpine/Dockerfile
  The implementation above is missing the cacerts necessary to connect to SSL/TLS
  resources, so updated the Dockerfile to address those issues.
  *NOTE* The OpenJDK versions and the versions used by this image match those outlined
  in JEPS 223 (http://openjdk.java.net/jeps/223)
* user-openjdk - informed by the deis example-java-jetty application found on github.
  https://github.com/deis/example-java-jetty
* alpine-openjdk - informed by the official alpine:8-jdk image found on docker.io.
  https://github.com/docker-library/openjdk/blob/master/8-jdk/alpine/Dockerfile
  *NOTE* The OpenJDK versions and the versions used by this image match those outlined
  in JEPS 223 (http://openjdk.java.net/jeps/223)
* user-openjdk - informed by the deis example-java-jetty application found on github.
  https://github.com/deis/example-java-jetty
@smebberson
Copy link
Owner

@rbellamy, thanks for this PR. Are you perhaps willing to maintain these images and keep versions up to date?

@rbellamy
Copy link
Contributor Author

Yes.

We're building our new platform, and deploying it via Docker in Kubernetes. My crystal ball says we'll be using Docker for the foreseeable future.

It's worth mentioning in the interest of full disclosure that I'm paying close attention to rkt and its architecture, so that's a future possibility. Even then, if we were to head in that direction, we'd most likely have a pretty large transition period where we'd be launching Docker containers in rkt, rather than moving wholesale into ACI.

@smebberson
Copy link
Owner

No worries, thanks for getting back to me. It'd be great if you could look after this image as best as you can while you're using it.

I'm just working on a few urgent fixes for alpine-base and then I'll get you to rebase, test and then I'll merge this in.

Copy link
Owner

@smebberson smebberson left a comment

Choose a reason for hiding this comment

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

This looks pretty good. You've followed suit on structure and documentation awesomely, thanks for that. I've just noted a couple of things... if you could get back to me on those, that'd be great :)

@@ -0,0 +1,31 @@
FROM smebberson/alpine-base
Copy link
Owner

Choose a reason for hiding this comment

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

Would you mind fixing this to a particular release? I started this way when doing these images, but I quickly ran into issues so now the rule is, they must be fixed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed pending push.

One question - should you be the MAINTAINER?


## Versions

- `8.92.14`, `latest` [(Dockerfile)](https://github.com/smebberson/docker-alpine/blob/alpine-openjdk-v8.92.14/alpine-openjdk/Dockerfile)
Copy link
Owner

Choose a reason for hiding this comment

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

Are you wanting to version these images based on the JDK version? I haven't really versioned any other images that way, simply because if I change a parent image, the version numbers get out of sync really easily and therefore I thought, what is the point...

Have a read of, https://github.com/smebberson/docker-alpine#versioning. Based on that. I'd suggest these images start at 1.0.0. What are your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed to follow convention, pending push.


## v8.92.14

- [smebberson/alpine-base: v3.0.0][smebbersonalpinebase300]
Copy link
Owner

Choose a reason for hiding this comment

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

As mentioned above, there is a new version of alpine-base that you can and should pin these images to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed to follow convention, pending push.

@@ -0,0 +1,22 @@
FROM smebberson/alpine-openjdk
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't need pinning, as it will only ever be built in development/testing.


RUN cd /app && mvn -V -U -B -DskipTests clean dependency:list install

CMD ["/app/start"]
Copy link
Owner

Choose a reason for hiding this comment

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

This convention is different than all other images here... start files are usually called run and go in /etc/services.d/{whatever}/... is there a reason for the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake. I've fixed to follow convention. Also fixed a bug in the Dockerfile:

  1. Use port 8081 rather than 5000 to avoid collision with locally-running registry
  2. Declare the exposed port.

* Update to use `alpine-base:v3.1.0`.
* Update `alpine-openjdk` versioning to follow convention.
* Fix `user-openjdk` to use the proper `s6` service run file convention.
* Fix `user-openjdk` to use port 8081 rather than 5000 to avoid collision with running local registry.
* Fix `user-openjdk` to expose port 8081.
@rbellamy
Copy link
Contributor Author

Updated per your comments. I the documentation expects tags based on OpenJDK v7.91.2 -> v1.0.0 and v8.92.14 -> v2.0.0.

@rbellamy
Copy link
Contributor Author

Hmmm - actually, the OpenJDK v7.91.2 won't have the user-openjdk fixes. Perhaps I should create a completely separate pull request with a completely clean tree, including those fixes in history? It's up to you.

@smebberson
Copy link
Owner

@rbellamy, thanks for the updates. I'm not too fussed about the examples, that will be fine. As long as they're there. I'll look to merge these in shortly.

@@ -1,4 +1,4 @@
FROM smebberson/alpine-openjdk
FROM terradatum/alpine-openjdk
Copy link
Owner

Choose a reason for hiding this comment

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

@rbellamy, did you mean to commit this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

did not... good catch!

G. Richard Bellamy added 2 commits October 29, 2016 18:27
* Fix `FROM` in `user-openjdk`
* Actually delete the unconventional `start` run file.
* Make sure the HelloWorld.java class launches Netty with the right port.
@rbellamy
Copy link
Contributor Author

Found two other modifications that didn't make it into my last set of commits. You should be good-to-go now.

@rbellamy
Copy link
Contributor Author

Also, you may have missed my question about the MAINTAINER - I've left it as you, since that seems to be the standard around here.

@smebberson
Copy link
Owner

@rbellamy I did miss that, sorry! I think I want to change... I do mostly maintain most of these but I would like to get more support. I was thinking we could update them to a new format, which I've actually already used in one container https://github.com/smebberson/docker-alpine/blob/master/alpine-base/Dockerfile#L2

Did you want to update the MAINTAINER for this new image to MAINTAINER Richard Bellamy (https://github.com/smebberson/docker-alpine)?

@rbellamy
Copy link
Contributor Author

Done.

I noticed that in #36 you discuss with @matthewvalimaki the possibility of creating a GitHub org - I think that's your best bet for facilitating more support. I'd be willing to help with PR's across the board...

@rbellamy
Copy link
Contributor Author

@smebberson gentle ping re: GitHub org.

@rbellamy
Copy link
Contributor Author

@smebberson ping re: GitHub org.

@rbellamy
Copy link
Contributor Author

Any idea when you'll be returning?

@smebberson
Copy link
Owner

@rbellamy, I have returned. Let's get this merged :) Is this PR still relevant in terms of versions used? Can we take a chat about a GitHub organisation offline?

@rbellamy
Copy link
Contributor Author

@smebberson it's still relevant. In fact I was just reviewing what I would need to do to bring the builds in house, so your timing is pretty good. And yes, I'd be happy to chat offline. Sent you an email re: Org.

@smebberson
Copy link
Owner

@rbellamy, I couldn't quite get this working... I made a couple of changes 69f7da6

Mainly updated the openjdk version (it's moved on in Alpine Linux), and tidied up the syntax a little. However, I think there is something wrong with the port mappings. In alpine-openjdk you're exposing 8081 but you're not using that in the ./run script for user-openjdk?

@smebberson
Copy link
Owner

@rbellamy, I just fixed the ports (https://github.com/smebberson/docker-alpine/commits/terradatum-alpine-openjdk) and can see that it is working now. Could you please just check my changes and make sure you're happy with them? If so, I'll merge and release....

@rbellamy
Copy link
Contributor Author

I'll be getting to this sometime this week

@smebberson
Copy link
Owner

@rbellamy, no worries. I look forward to merging this in :)

@rbellamy
Copy link
Contributor Author

Nice work @smebberson! Looks good and runs as expected for me.

curl -X GET http://localhost:8081 returns:

Powered by Deis
Release unknown on 95be51c28c43

@rbellamy
Copy link
Contributor Author

rbellamy commented Mar 1, 2017

@smebberson - I did send you two private emails about the GitHub Organization - I'd like to get something off the ground ASAP if you're still interested.

@smebberson
Copy link
Owner

@rbellamy, merged and released :) https://hub.docker.com/r/smebberson/alpine-openjdk/

@rbellamy
Copy link
Contributor Author

Um... not according to GitHub. Looks open and conflicted to me... :(

I just fixed the merge conflict so you should be able to go forward with the PR merge.

Let's please have that conversation about the GitHub Organization - is the email you have published in your GH profile the correct one? Regardless, the email I have listed is valid.

I really, really really get it how slammed and time constrained you must be - which is EXACTLY the reason the awesome work you've done here could use the kind of community support an Org would provide.

Please reach out to me ASAP.

@smebberson smebberson merged commit da17670 into smebberson:master Mar 16, 2017
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.

2 participants