-
Notifications
You must be signed in to change notification settings - Fork 126
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 documentation to build the site with a docker container #148
Conversation
585db3c
to
be63d99
Compare
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): Jenkins built the site at commit id 585db3c with Jekyll and staged it here. Happy reviewing. Note that any previous site has been deleted. This staged site will be automatically deleted after its TTL expires. Push any commit to the pull request branch or re-trigger the build to get it staged again. |
Refer to this link for build results (access rights to CI server needed): Jenkins built the site at commit id be63d99 with Jekyll and staged it here. Happy reviewing. Note that any previous site has been deleted. This staged site will be automatically deleted after its TTL expires. Push any commit to the pull request branch or re-trigger the build to get it staged again. |
Thank you. I like the idea. I will defer the review to @davorbonaci since I am not very familiar with jekyll. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a great improvement @sb2nov, and it will certainly simply this for a ton of people!
Left a few minor comments about comments ;-)
README.md
Outdated
@@ -56,6 +56,16 @@ get tricky, so please leave this directory out of your commits and pull request | |||
|
|||
The committer doing the final merge will generate the `content/` directory at that time. | |||
|
|||
### Running using Docker | |||
|
|||
**Note:** This is an optional method if you don't want to install gems locally and just use a container for development. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... use a container for running Jekyll. Mention it is platform-specific, i.e., you cannot use this on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should work on windows as now docker natively supports it. https://docs.docker.com/docker-for-windows/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you have a Bash script also? Running that on Windows is ~possible, but painful to the degree you'd probably want to install Ruby + gems instead. SGTM either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
README.md
Outdated
|
||
**Note:** This is an optional method if you don't want to install gems locally and just use a container for development. | ||
|
||
Make sure you have docker installed and we'll use the official jekyll image to build the site. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure you have Docker installed on your local machine.
docker -> Docker
jekyll -> Jekyll
Move the image piece in the serve
paragraph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -0,0 +1,35 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments at the top. What it does, how to use it, what it assumes, etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
run_with_docker.sh
Outdated
@@ -0,0 +1,35 @@ | |||
#!/bin/bash | |||
|
|||
serve() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plenty of duplication between serve & test functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
run_with_docker.sh
Outdated
--env="BUNDLE_CACHE=true" \ | ||
--env="BUNDLE_PATH=/srv/jekyll/vendor/bundle" \ | ||
jekyll/jekyll \ | ||
bash -c "bundle config build.nokogiri --use-system-libraries && bundle install && bundle exec jekyll server --force_polling --watch -H 0.0.0.0 -P 4000 --incremental" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
serve vs. server here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Thanks for the comments. @davorbonaci Should I add it to the contributor guide or just leave it in the Readme ? |
I'd probably do it here only. |
247c80e
to
f1b10b2
Compare
Thanks for the feedback, updated the PR. |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): Jenkins built the site at commit id 247c80e with Jekyll and staged it here. Happy reviewing. Note that any previous site has been deleted. This staged site will be automatically deleted after its TTL expires. Push any commit to the pull request branch or re-trigger the build to get it staged again. |
Refer to this link for build results (access rights to CI server needed): Jenkins built the site at commit id f1b10b2 with Jekyll and staged it here. Happy reviewing. Note that any previous site has been deleted. This staged site will be automatically deleted after its TTL expires. Push any commit to the pull request branch or re-trigger the build to get it staged again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful!
README.md
Outdated
|
||
Prerequisites: Make sure you have Docker installed on your machine. | ||
|
||
For building the site use the command `./run_with_docker.sh server`. This will start the webserver inside a Docker container and port forward to your local machine at `localhost:4000`. We'll use the official Jekyll image to build the site. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For building the site*,* use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
README.md
Outdated
|
||
For building the site use the command `./run_with_docker.sh server`. This will start the webserver inside a Docker container and port forward to your local machine at `localhost:4000`. We'll use the official Jekyll image to build the site. | ||
|
||
For running the tests to verify all links use `./run_with_docker.sh test`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For running the website tests, use...
Remove "to verify all links". Tests currently just do that, but soon enough they'll be doing more stuff. So, just abstract away what the tests does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, updated.
run_with_docker.sh
Outdated
# This script will run Jekyll inside Docker to build the site. | ||
# | ||
# Use "./run_with_docker.sh server" to build the site at localhost:4000. | ||
# Use "./run_with_docker.sh tesst" to test for any broken links. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, thanks for catching this. Should have been more careful.
f1b10b2
to
de3cc57
Compare
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): Jenkins built the site at commit id de3cc57 with Jekyll and staged it here. Happy reviewing. Note that any previous site has been deleted. This staged site will be automatically deleted after its TTL expires. Push any commit to the pull request branch or re-trigger the build to get it staged again. |
Adding documentation in the Readme to allow building the site with a docker container. This solves some of the pain we face with ruby versions and gems being installed globally etc. So should make it easier for people to updated the docs.
Created a helper script for the two user facing functions, if you think this is reasonable I can also add it to the Contributor guide page.
R: @davorbonaci @aaltay PTAL