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 Dockerfile to support running in a container #135

Merged
merged 7 commits into from
Nov 11, 2018

Conversation

bkeepers
Copy link
Contributor

@bkeepers bkeepers commented Oct 2, 2018

Motivation

Between Licensee and Linguist, there are a lot of dependencies needed to get repolinter working to its fullest potential.

Proposed Changes

This PR adds a Dockerfile which builds both the Ruby and Node dependencies.

Usage:

$ docker run -it -v $PWD:/src -w /src bkeepers/repolinter
Target directory: /src
Ruleset: repolint.json
Linguist Axiom: Linguist not found in path, only running language-independent rules
✔ license-file-exists: found (LICENSE)
✔ readme-file-exists: found (README.md)
✔ contributing-file-exists: found (CONTRIBUTING.md)
✔ code-of-conduct-file-exists: found (CODE_OF_CONDUCT.md)
✔ readme-references-license: File README.md contains license
✔ binaries-not-present: Excluded file type doesn't exist (**/*.exe,**/*.dll,!node_modules/**)
✔ test-directory-exists: found (script/test)
✔ integrates-with-ci: found (.github/main.workflow)
✔ code-of-conduct-file-contains-email: File CODE_OF_CONDUCT.md contains email address
⚠ github-issue-template-exists: not found: (ISSUE_TEMPLATE*, .github/ISSUE_TEMPLATE*)
⚠ github-pull-request-template-exists: not found: (PULL_REQUEST_TEMPLATE*, .github/PULL_REQUEST_TEMPLATE*)
✔ license-detectable-by-licensee: Licensee identified the license for project: MIT

Test Plan

Ideally, you could build and run the tests via Docker on Travis CI.

TODO:

  • Reduce the size of the built image
  • Docs on using via docker

@bkeepers bkeepers requested a review from trevmex as a code owner October 2, 2018 16:15
@craigez
Copy link
Contributor

craigez commented Oct 2, 2018

I was just thinking last week this would be useful, but hadn't had a chance to look into creating the dockerfile yet.

I tested this out and it works for me. Tested both by pulling the image from dockerhub or building it directly from the dockerfile. By no means am I a docker expert though.

One issue, I don't think it installs linguist in the expected way for repolinter.

Linguist Axiom: Linguist not found in path, only running language-independent rules

From a quick look at the code and running the shell on the docker image, maybe it is expecting it installed as "linguist" when it's installed by this docker file as "github-linguist".

@bkeepers
Copy link
Contributor Author

bkeepers commented Oct 2, 2018

From a quick look at the code and running the shell on the docker image, maybe it is expecting it installed as "linguist" when it's installed by this docker file as "github-linguist".

Looks like this was recently changed: github-linguist/linguist#4208

@dmuth dmuth mentioned this pull request Oct 2, 2018
@bkeepers
Copy link
Contributor Author

bkeepers commented Oct 2, 2018

Fixed for linguist command in c13fe49. I'll open a separate PR for that since it needs fixed anyway.

@jwsloan
Copy link
Contributor

jwsloan commented Oct 2, 2018

The multi-stage build is a really slick way of dealing with needing ruby and node. I have a Dockerfile branch of this repo I haven't pushed yet, and I was using asdf to install both. Either way, I'm happy to see other folks see the value of having a Dockerfile in projects like this.

Going about adding Dockerfiles may be what I do for #hacktoberfest...

@bkeepers
Copy link
Contributor Author

bkeepers commented Oct 2, 2018

  • Reduce the size of the built image

I got this down to 181MB. I think that's pretty good considering all the dependencies (ruby & node runtimes, git, gems & npm packages).

  • Docs on using via docker

I added some docs in the Dockerfile. If you want to publish a docker image as part of the release process, it might also be worth adding it in the README.

@@ -0,0 +1,41 @@
# To build:
Copy link
Contributor

Choose a reason for hiding this comment

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

This would probably be good to have in the top level README as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, though I think it is good enough to merge for now.

@jwsloan
Copy link
Contributor

jwsloan commented Oct 2, 2018

It might be cool to push an image from this out to DockerHub. Then anyone could docker pull todogroup/repolinter and then run it.

@bkeepers
Copy link
Contributor Author

bkeepers commented Oct 2, 2018

Looks like I got a little too aggressive with pruning dependencies:

$ docker run -it --entrypoint '' -v $PWD:/src -w /src bkeepers/repolinter github-linguist
Traceback (most recent call last):
	13: from /usr/local/bundle/bin/github-linguist:23:in `<main>'
	12: from /usr/local/bundle/bin/github-linguist:23:in `load'
	11: from /usr/local/bundle/gems/github-linguist-7.0.0/bin/github-linguist:5:in `<top (required)>'
	10: from /usr/local/lib/ruby/site_ruby/2.5.0/rubygems/core_ext/kernel_require.rb:59:in `require'
	 9: from /usr/local/lib/ruby/site_ruby/2.5.0/rubygems/core_ext/kernel_require.rb:59:in `require'
	 8: from /usr/local/bundle/gems/github-linguist-7.0.0/lib/linguist.rb:1:in `<top (required)>'
	 7: from /usr/local/lib/ruby/site_ruby/2.5.0/rubygems/core_ext/kernel_require.rb:59:in `require'
	 6: from /usr/local/lib/ruby/site_ruby/2.5.0/rubygems/core_ext/kernel_require.rb:59:in `require'
	 5: from /usr/local/bundle/gems/github-linguist-7.0.0/lib/linguist/blob_helper.rb:2:in `<top (required)>'
	 4: from /usr/local/lib/ruby/site_ruby/2.5.0/rubygems/core_ext/kernel_require.rb:59:in `require'
	 3: from /usr/local/lib/ruby/site_ruby/2.5.0/rubygems/core_ext/kernel_require.rb:59:in `require'
	 2: from /usr/local/bundle/gems/charlock_holmes-0.7.6/lib/charlock_holmes.rb:1:in `<top (required)>'
	 1: from /usr/local/lib/ruby/site_ruby/2.5.0/rubygems/core_ext/kernel_require.rb:59:in `require'
/usr/local/lib/ruby/site_ruby/2.5.0/rubygems/core_ext/kernel_require.rb:59:in `require': libicudata.so.57: cannot open shared object file: No such file or directory - /usr/local/bundle/gems/charlock_holmes-0.7.6/lib/charlock_holmes/charlock_holmes.so (LoadError)                                                                                                                                                    

@bkeepers
Copy link
Contributor Author

bkeepers commented Oct 3, 2018

The error ☝️ has been fixed.

Copy link
Collaborator

@trevmex trevmex left a comment

Choose a reason for hiding this comment

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

I like it! Just downloaded your fork and ran it. Works great!!!

@trevmex trevmex merged commit 1c3500f into todogroup:master Nov 11, 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.

4 participants