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

Adds Rocket.Chat #888

Merged
merged 1 commit into from Aug 13, 2015
Merged

Adds Rocket.Chat #888

merged 1 commit into from Aug 13, 2015

Conversation

pierreozoux
Copy link
Contributor

@psftw
Copy link
Contributor

psftw commented Jul 14, 2015

It doesn't look like you are involved with upstream ( ?), but they have a Dockerfile in their repository, so it is worth reaching out to them if you haven't. The 0.4.0 release on GitHub is marked as "pre-release". I would be hesitant to publish this before a more "release" release is ready, and the project itself appears to be about two months old which also makes me hesitant. There are a bunch of things that are either not implemented or are not covered here -- admin account, external authentication, and email integration come to mind. That being said, the app worked for me. nitpick: The first WORKDIR line could be removed and replaced with cd ... && in the next line to save a layer. I wonder if it would make sense to set a default ROOT_URL for users and then talk about overriding it in the documentation for non-standard setups. I would imagine in most real-world deployments you'd put this behind a reverse proxy that handles TLS.

@pierreozoux
Copy link
Contributor Author

involved in upstream

I am not a public member actually, you are right (I'm a private member of the repo). But as an IndieHosters, I'm interested in software delivering nice Dockerfiles :) And actually, this project is my first try to lobby developers having something nice, and the team responded with the even better idea to submit it as an official one! I might also submit other official images, so I took the opportunity to make it with them!
If you want some 'official' acknowledgment, I'll poke the team to comment here :)

pre-release

Actually this is the first release that is made for the Dockerfile, but if you prefer a release more release, I can wait the next one, and update my PR.

About the 2months old project, I'll let the project owner defend :) I think it is a great piece of software already, but I have to admit, I don't know your 'policy' about which FOSS project to accept or not?

Dockerfile advices

WORKDIR vs cd

I used WORKDIR for clarity, but yes, you are right a cd would have saved a layer. One less layer is always greener!

ROOT_URL

Yes, great idea! This would save one more configuration, and it would work out of the box with something like localhost. Really good idea!

What's next

I'll let the project owner respond here to the points, and once there is a release more release, I'll update the PR with your advices.

Best!

Pierre

@engelgabriel
Copy link

Hi @psftw , thanks for the quick responce.

involved in upstream

First thanks, thanks @pierreozoux and @Sing-Li for the great work. Their are active members of the team indeed.

pre-release

I've just marked as such, because the new settings UI is not ready. But the app is fully functional and tested in production deployments. It is just missing some functionality we believe we should have implemented before an official launch. The project is actually almost a year old, but it was a closed project of my company. We have clients using the chat in production with over 1k users for about 8 months.

what's next

We have a really long roadmap ahead of us, with REST APIs, native client apps, better WebRTC support, etc.

@yosifkit
Copy link
Member

@engelgabriel, thanks for the response.

@pierreozoux, I have just one tiny nitpick; is there a reason for a specific version of node? If you use node:0.10, the image will get rebuilt each time node 0.10 gets a minor version bump. Otherwise it will cause extra work to bump the node version and then make a PR here to update the git commit references. But, I understand if that is an important part of your software/testing to be tied to a specific version of node 0.10.

@pierreozoux
Copy link
Contributor Author

@yosifkit from what I see around about meteor hosting, I think it is better to stick to the node version used by meteor. To be honest I never hit any issues yet by doing so, but on the web, I can see people having troubles not doing so.

@pierreozoux
Copy link
Contributor Author

Ok, I updated my PR (on the docs also) to reflect your recommendations regarding less layers and ROOT_URL.

Please let me know if you see anything else!

Best!

@psftw
Copy link
Contributor

psftw commented Jul 15, 2015

I don't know your 'policy' about which FOSS project to accept or not

The closest we have to a policy about the subjective requirements is this snippet from https://docs.docker.com/docker-hub/official_repos/

There are also subjective considerations during the review process. These subjective concerns boil down to the basic question: “is this image generally useful?” For example, the python Official Repository is “generally useful” to the large Python developer community, whereas an obscure text adventure game written in Python last week is not.

I would love to be more objective about what makes a good candidate, and I think the above quote is too broad, but I haven't really figured it out in my mind yet, let alone lobbied the group for my vision. It's definitely something I've been thinking about lately.

I won't object to releasing with 0.4.0, because the Dockerization LGTM and it functions well, though I feel like the current documentation is a bit lacking. Take email integration for instance. The initial account creation asks for an email, and then it doesn't appear to do anything with it. If there is in fact email integration, I wasn't able to find documentation on how to configure it -- granted I only skimmed the upstream repo.

@pierreozoux
Copy link
Contributor Author

< my 2 cents - digression >
I have the feeling (and I think it is partly the purpose) that docker could be a great (the last?) webapp store.
It is a great way to distribute packaged app, and I hope app developers will go toward this more and more.

And then it depends if it is about library or webapp. The name of the repo is library but it includes wordpress, which is not really a library.

Maybe one solution would be to have a just library repo (with redis, mysql...) and another kind of webapp store where you'll even find the text adventure game.
(Maybe a good candidate for the docker-compose repo idea docker/hub-feedback#57)

There is this blog post and my answer about this question somehow: https://fhackts.wordpress.com/2015/02/09/public-virtual-appliance-repositories-docker-vs-turnkeylinux-vs-vmware-which-is-the-largest-community/?hc_location=ufi
</ digression >

About the email, I have to admit, I'm not enough into the app to know these details. @engelgabriel could you answer please? (I'm also interested actually!)

@engelgabriel
Copy link

Hi, we literally just merged RocketChat/Rocket.Chat#304

There will be a link for admins to go to the page and configure everything about the app, including name and logo in the near future.

Once I finish the UI, I'll create the appropriate wiki pages with screenshots and explanations.

@engelgabriel
Copy link

All the UI fields as mapped to ENVIRONMENT_VARIABLES, so it can be configured by deployment scripts just as easily. Although, ENVIRONMENT_VARIABLES can only set the INITIAL value... once the values are copied to the DB, the DB value has priority over ENVIRONMENT_VARIABLES. We may create a --force or SETTINGS_LOAD_ORDER later to change the default behaviour.

@psftw
Copy link
Contributor

psftw commented Jul 15, 2015

@pierreozoux thanks for the feedback. I tend to agree. A distinction between images that live in a generic problem space (data stores, language runtimes, etc.) and images that represent solutions to specific problems is a reasonable approach. For right now, we just have a single "Official" status to work with. As for compose integration in DockerHub, I don't have any official word on it, but I think it is a matter of time.

@tianon
Copy link
Member

tianon commented Aug 12, 2015

Dockerization looks sane to me; my only comment would be that FROM node:0.10.39 should probably just be FROM node:0.10 so that you don't have to make manual changes to take advantage of new node updates. 👍

@pierreozoux
Copy link
Contributor Author

@tianon updated :)

@tianon
Copy link
Member

tianon commented Aug 13, 2015

LGTM 👍

I agree this is definitely on the fence for the program, but it's not unlike things we already have, and upstream is obviously heavily involved, so 🎉 in my book.

@yosifkit
Copy link
Member

LGTM, Build test of #888; 68cd064 (rocket.chat):

$ url="https://raw.githubusercontent.com/docker-library/official-images/68cd064b33ac1c7f23145577eede8f3e337992f4/library/rocket.chat"
$ bashbrew build "$url"
Cloning rocket.chat (git://github.com/RocketChat/Docker.Official.Image) ...
Processing rocket.chat:0.4.0 ...
Processing rocket.chat:0.4 ...
Processing rocket.chat:0 ...
Processing rocket.chat:latest ...
$ bashbrew list --uniq "$url" | xargs test/run.sh
testing rocket.chat:0.4.0
    'utc' [1/3]...passed
    'cve-2014--shellshock' [2/3]...passed
    'no-hard-coded-passwords' [3/3]...passed

yosifkit added a commit that referenced this pull request Aug 13, 2015
@yosifkit yosifkit merged commit 2b303cf into docker-library:master Aug 13, 2015
@engelgabriel
Copy link

👍 😄 🎉 🎈

Just to share: the Federal University in Brazil is deploying Rocket.Chat nationally to be the main communication platform for 40k users on a pilot project, all using Docker Containers, so the timing could not be better!

And we submitted today our Mobile Apps to Google Play and Apple Store, so this is an amazing week for us!

Thanks guys!

@md5
Copy link
Contributor

md5 commented Aug 13, 2015

@engelgabriel 🤘

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.

None yet

7 participants