-
Notifications
You must be signed in to change notification settings - Fork 356
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
Dockerfile (with some convenience Makerules) for development, testing and serving #461
Conversation
deeped hierarchy isn't really necessary 😉
- Use `make image` to build the github-pages image - Use `make shell` to produce a bash shell - run the scripts inside that shell by simply running - `./script/bootstrap`, usually run this first to ensure a fresh `bundle install` - `./script/cibuild` - `./script/test-site` - `./script/fmt` - Run the server by: - exporting `SITE` first and then calling `make server`, e.g.: `export SITE=../path-to-jekyll-site; make server` - prepending an assignment to `SITE` to the `make server` rule, e.g: `SITE=../path-to-jekyll-site make server`
This is great; I'll close my pr :) |
@vidbina this is great. I'd love to get this merged. Would you be able to add documentation? If so, I believe this should be big improvement. |
Dockerfile
Outdated
WORKDIR /gh-pages | ||
|
||
RUN bundle config local.github-pages /gh-pages | ||
RUN bundle install |
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.
We may need to modify the Gemfile to add the Gem to the jekyll_plugins
group so that it can hook earlier into the bootstrap process and set defaults, e.g., gem "github-pages", group: :jekyll_plugins
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'm not sure I totally understand what you mean with this, please elaborate and I'll try to get on it asap 😉.
My GitHub Pages projects do contain the gem "github-pages", group: :jekyll_plugins
line, but then I believe I should then probably fire up "bundle exec jekyll server ..." instead of just "jekyll serve ...".
Is there any logic performed on the jekyll_plugins
group? Is it not only there for organization?
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.
Is there any logic performed on the jekyll_plugins group? Is it not only there for organization?
The Jekyll Plugins group allows the GitHub Pages Gem to get bootstrapped earlier in Jekyll's process and set configuration defaults and constraints to match GitHub Pages. I'm struggling to load a gemspec into a gem group, so we may need to create a new / modify the Gemfile for docker.
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.
Have you guys figures this out in the end? It seems to me that jekyll in the container ignores github-pages entirely and just uses its own default configuration, making this whole make server
and contrib/func.sh
thing rather useless. :-/
@johndmulhausen and @benbalter can you check if the documentation I added makes any sense at all?!? Thanks in advance 🙇 |
This image looks very exciting for allowing local testing of GitHub Pages sites. I've been testing it on the GitHub Pages examples and I've noticed some examples where it produces errors. All of these were produced by cloning the site and then running
I'm not sure if this is what's causing the above errors, but it looks like the way Jekyll has been set up doesn't match how GitHub Pages does rendering: Pages runs Jekyll in 'safe mode' with a specific whitelist of plugins, so sites which work in this container might not work on Pages if they're using custom plugins. I've found the settings for all of this in |
Hey @wolfgang42, thanks for this. I'll take some time soon enough to look into this issue. I've been flooded with some other chores meanwhile so I'll get back at you as soon as I know more 😉. Thanks for listing the specific examples and the errors they produce. |
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.
Wow! I really, really like this! I added a few requests below, but I'd definitely be happy merging this. The demo really shows it off beautifully. 🎉 ❤️ Very well done.
Dockerfile
Outdated
git \ | ||
make | ||
|
||
COPY . /gh-pages |
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.
Due to how Jekyll.sanitized_path
works, let's put this in a subdirectory 2 levels deep, e.g. /gh/pages
or /src/gh-pages
. Otherwise, if you have a file called gh-pages.html
, you'll get an error. It's confusing so let's tackle that before merging.
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.
Something like cbe4cdf?
Makefile
Outdated
# exposing it using `expose SITE="../path-to-jekyll-site"` prior to calling or | ||
# by prepending it to the make rule e.g.: `SITE=../path-to-site make server` | ||
server: | ||
ls "${SITE}" >/dev/null && \ |
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.
Can an error message be printed here if the SITE variable doesn't exist?
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.
Will an echo
suffice? 03709aa
Makefile
Outdated
-p 4000:4000 \ | ||
-v ${PWD}:/gh-pages \ | ||
-v `realpath ${SITE}`:/site \ | ||
-w /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.
Can we use /src/site
(a subdirectory 2 levels deep) for reasons stated above?
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 😉 9d8fe18
README.md
Outdated
**Note:** the `github-pages` function may be enabled by sourcing func.sh. This can be done by appending | ||
|
||
```bash | ||
source $PATH_TO_THIS_DIRECTORY/func.sh |
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.
This is great! Can it go in a contrib/
subdirectory instead of the root of the project? contrib/docker
maybe?
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 😉 f4a8d87
In response to github#461 (comment)
@parkr it took forever for me to get around to this, apologies. It's only now when I'm chilling on an island 🌴 that I can sit for a few minutes and catch up on items on my to-do list. I still intent to address the issues pointed out by @benbalter and @wolfgang42 but it could take a little while. Just bear with me and rest assured that I've heard and acknowledged the concerns loud and clear 👂 😉 |
@wolfgang42 just called
Not sure why I doesn't evaluate the the erb template. Will look into it. Running
for which I initially figured that it could be related to the old For facebook/react I haven't found a jekyll site. Perhaps the link you referenced doesn't point to the static page for React? Will have to do a bit of digging tomorrow to figure out what's up but perhaps @benbalter or @parkr have some insights that could nudge me in the right direction. |
@vidbina It looks like the React Jekyll site was deleted last month: facebook/react#10941, facebook/react@887ccf2 |
Hey @wolfgang42. The Jekyll site now builds as long as you use the
The invalid byte sequence issue was resolved by configuring the locale of the container through the setting of the As for the execjs issue, that was resolved by Let me know if that solves the issues and bump @parkr and @benbalter to take it from there once you're convinced that it works well enough 😉. |
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.
Hey @benbalter, I've more or less addressed every issue you highlighted. Let me know if this suffices, otherwise let me know what still needs to be improved. ✌️
@johndmulhausen started this with #423 which led to a subsequent PR into his set of changes johndmulhausen#1. Since his
master
branch is lagging, the diff contains a lot of noise (commits that are unrelated to the Dockerization things) as pointed out by @benbalter. This PR minimizes the diff to something more suited for scrutiny.Changes
Usage
make image
to build the github-pages imagemake shell
to produce a bash shell./script/bootstrap
, usually run this first to ensure a freshbundle install
(note that with themake shell
rule mounting the pages-gem directory into the container's/gh-pages
path (the bundler override for the github-pages gem), one may test changes made to the directory on the host, rather than being limited to having to rebuild the image on every file change 😉)./script/cibuild
./script/test-site
./script/fmt
SITE
to themake server
rule, e.g:SITE=../path-to-jekyll-site make server
SITE
first and then callingmake server
, e.g.:export SITE=../path-to-jekyll-site; make server
. After theexport
, one may trigger the server by simply runningmake server
, but theSITE
assignment becomes less explicit. When you change to another terminal that perhaps already has the variableSITE
defined, perhaps even for other purposes, one will basically be altering the terminal's state.Demo