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

improvements for docker build #5

Merged
merged 5 commits into from
Jul 11, 2016
Merged

improvements for docker build #5

merged 5 commits into from
Jul 11, 2016

Conversation

multiwave
Copy link

I used your Dockerfile as a base, key changes:

  • to run it you now use REDIS_URL instead of 3 env vars, and HTTP_PATH env var is now optional (default being '/qless') example: docker run -e REDIS_URL="redis://127.0.0.1:6379/0" $IMG_NAME bundle exec rackup qless.ru -o0.0.0.0 -p 5678
  • Dockerfile is according to dockers best practices (fewer RUN commands)
  • final image is about 110 MiB smaller than ubuntu based prior version
  • jquery is no longer loaded from CDN (in our environment that doesn't work, client computers interacting with cluster have no internet access) bug Do not use CDN versions of JavaScript scripts qless#244 references that problem too

I hope some of those changes make into your master.

map(ENV['HTTP_PATH']) { run Qless::Server.new(client) }
gui_path = '/qless'
if ENV['HTTP_PATH']
gui_path = ENV['HTTP_PATH']
Copy link
Contributor

Choose a reason for hiding this comment

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

You can turn this whole block into:

gui_path = ENV.fetch('HTTP_PATH', '/qless')

@weiser
Copy link
Contributor

weiser commented May 25, 2016

@multiwave thanks for sending the submission!

I like the idea of using REDIS_URL. I think, though, that the user should be able to provide either REDIS_URL or the other env vars because otherwise we'll introduce breaking change to groups which upgrade this without noticing the env var change.

So, I'm happy to merge this provided:

  1. README.md is updated to show how to run the container using either REDIS_URL or the other env vars.
  2. Update the implementation of qless.ru to use either REDIS_URL or the other env vars.

@@ -0,0 +1,13 @@
diff --git a/lib/qless/server/views/layout.erb b/lib/qless/server/views/layout.erb
Copy link
Contributor

@weiser weiser May 25, 2016

Choose a reason for hiding this comment

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

I think that this change should happen in seomoz/qless, not here. Qless, here, can be bumped once seomoz/qless has updated.

@multiwave
Copy link
Author

I guess it should be okay now, about the javascript patch - I will keep it here, as I cannot wait for upstream to fix it. But it is easy to remove, do not merge local_js.patch and skip the last lines in Dockerfile.

@weiser
Copy link
Contributor

weiser commented Jun 6, 2016

Re: the java script patch. Could you please add a reference to the qless issue that is necessitating the JS patch so we know when the patch can be removed and qless updated?

@multiwave
Copy link
Author

Sorry for the late response, didn't realize there was feedback. I have now referenced the javascript Issue in comment right above the patch. But when they fixed it upstream the patch will fail, therefore Dockerfile build fails, therefore you know it can be removed ;)

@weiser
Copy link
Contributor

weiser commented Jul 11, 2016

@multiwave thanks for getting back to us on this. Sorry I'm so late at getting this (I was on vacation).

Merging.

Thanks, again, for contributing,

@weiser weiser merged commit 59a8d05 into seomoz:master Jul 11, 2016
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