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

Add lightstreamer-server image #1342

Merged
merged 6 commits into from
Apr 1, 2016
Merged

Add lightstreamer-server image #1342

merged 6 commits into from
Apr 1, 2016

Conversation

gfinocchiaro
Copy link
Contributor

Proposal for adding a Docker image of Lightstreamer Server.

@gfinocchiaro
Copy link
Contributor Author

Hi,

any progress on this request?

Thanks
Gianluca

@psftw
Copy link
Contributor

psftw commented Jan 21, 2016

Thanks for submitting this. I won't be able to dig deeper today, but there are some changes that could be made based upon a quick read of the Dockerfile: Downloaded artifacts should be verified, as mentioned in the documentation. The three RUN commands could be merged to simplify the layers a bit. The JAVA_HOME environment variable is already set in the parent java image so that can be removed. Is there a good reason for switching the listening port from 8080 to 80 other than convenience? I wonder what a production environment would look like -- perhaps a mention in the docs on configuring TLS would help users trying to deploy this since I presume a front-end proxy could complicate things (?).

META: Should we call this lightstreamer-server or perhaps lightstreamer to keep it a bit shorter? I take it you didn't intend to put a link to the kaazing repository in your maintainer line?

@gfinocchiaro
Copy link
Contributor Author

Thanks for your prompt reply, really appreciated! Please see my inline replies:

Downloaded artifacts should be verified, as mentioned in the documentation

Ok, we are going to upload our public key as described.

The three RUN commands could be merged to simplify the layers a bit.

Ok.

The JAVA_HOME environment variable is already set in the parent java image so that can be removed.

Ok

Is there a good reason for switching the listening port from 8080 to 80 other than convenience?

No, we will keep factory setting as per your suggestion.

I wonder what a production environment would look like -- perhaps a mention in the docs on configuring TLS would help users trying to deploy this since I presume a front-end proxy could complicate things (?).

If you mean the docs to upload under docker-library/docs, I can try to provide with some further details.

META: Should we call this lightstreamer-server or perhaps lightstreamer to keep it a bit shorter?

Ok, "lightstreamer" is good too for us.

I take it you didn't intend to put a link to the kaazing repository in your maintainer line?

Right...

In the next few day I'll commit an updated Dockerfile and get back to you.

Thanks a lot!
Gianluca

@gfinocchiaro
Copy link
Contributor Author

Hi Peter,

just updated Dockerfile as per your suggestions, included security check of downloaded artifacts
Please let me know how to go on.

Thanks
Gianluca

@psftw
Copy link
Contributor

psftw commented Feb 2, 2016

Thanks for the update. This looks good to me and I don't see any blockers at this point. I usually spin up an instance and test out the functionality, but having broken my wrist over the weekend, I may not get to that. I noticed that this is a demo version limited to 20 users. Would paying customers with licenses be able to use this same image + license, or would there be a different install path for them? Also, is there a way to disable the extra logging to /lightstreamer/logs?

@gfinocchiaro
Copy link
Contributor Author

Hi Peter,

I'm sorry for you wrist, I hope that you will get well soon.

Yes, paying customers may use the same image, but it also required to update a configuration with relative license information (for example, a pointer to the license file).

What do you mean exactly for "extra logging"?

@psftw
Copy link
Contributor

psftw commented Feb 3, 2016

What do you mean exactly for "extra logging"?

There are log files written in /lightstreamer/logs within the container, and log data is also streamed out of the process directly. This isn't a big deal, but in an ideal world, I would only have log output to one place. Logging to files within the container will be less performant (unless using a volume), and logging to stdout/err is the preferred route. Something to consider if there is flexibility in how lightstreamer can handle log data.

@gfinocchiaro
Copy link
Contributor Author

I have updated Docker file with command to replace logging configuration in order to log only to stdout.
In the documentation we could show how to mount a custom configuration to log on a volume

Thanks
Gianluca

@gfinocchiaro
Copy link
Contributor Author

Hi,

is there anything else I can do on the Docker file?

@tianon
Copy link
Member

tianon commented Feb 26, 2016

@gfinocchiaro sorry for the delay! I've taken a look, and thanks to @psftw's initial suggestions, I'm really happy with where this is at now; the Dockerfile looks good (although I haven't tried building/running just yet)

Do you have a PR for the docs repo in-progress somewhere I could peek at? (https://github.com/docker-library/docs/blob/master/README.md)

@gfinocchiaro
Copy link
Contributor Author

Hi,

no problem for the delay! Actually, it was not clear to me the process for building the docs. I'll dive deeper into the README.md and get back to you.
I just wanted to be sure that no other issues due to the Docker file could affect the final publication.

Thanks a lot!
Gianluca

@tianon
Copy link
Member

tianon commented Feb 26, 2016 via email

@gfinocchiaro
Copy link
Contributor Author

Hi,

I have two questions:

  1. I was not able to find any specification about the logo.png file to add: the mentioned url link seems not to give any info on this.
  2. An attempt to run
$ ./update.sh lightstreamer-server

generates an error like the following:

‘.template-helpers/template.md’ -> ‘lightstreamer-server/README.md’
TAGS => generate-dockerfile-links-partial.sh
curl: (22) The requested URL returned error: 404 Not Found

Where am I wrong?

Thanks a lot.
Gianluca

@psftw
Copy link
Contributor

psftw commented Feb 29, 2016

@gfinocchiaro

logo - put in a PR to clarify that there isn't really a specific set of hard requirements for logo.png
update.sh - errors for this are expected prior to the repository getting accepted, no worries

@tianon
Copy link
Member

tianon commented Feb 29, 2016 via email

@gfinocchiaro
Copy link
Contributor Author

Ok, I'll do that.
Thanks!

@gfinocchiaro
Copy link
Contributor Author

Hi,

here the PR for the docs.

@tianon
Copy link
Member

tianon commented Mar 30, 2016

Ok, took another look today, and only have three minor comments:

  1. gpg --verify should be replaced by gpg --batch --verify (see Fix suggested "gpg" usage to stop relying on deprecated and insecure behavior #1420 for more details for why)
  2. line 25 probably ought to remove the .asc file as well (rm Lightstreamer.tar.gz)
  3. ENTRYPOINT probably ought to be CMD (see https://github.com/docker-library/official-images/blob/master/README.md#consistency)

Other than that, I think we're looking great here -- if you'd like, I'd be happy to make a PR to your repo for these few minor changes. 👍

@gfinocchiaro
Copy link
Contributor Author

Ok, I've just updated Dockerfile.

Let me know.

@yosifkit
Copy link
Member

yosifkit commented Apr 1, 2016

LGTM, Build test of #1342; 5eeb121 (lightstreamer):

$ bashbrew build "lightstreamer"
Cloning lightstreamer (git://github.com/Lightstreamer/Docker) ...
Processing lightstreamer:latest ...
Processing lightstreamer:6.0.1_20150730 ...
$ bashbrew list --uniq "$url" | xargs test/run.sh
testing lightstreamer:latest
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...passed

@yosifkit yosifkit merged commit 780bc14 into docker-library:master Apr 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants