timwis / jkan

A lightweight, backend-free open data portal, powered by Jekyll
https://jkan.io
MIT License
218 stars 310 forks source link

Fix docker spin up steps (#2) #254

Closed lydiascarf closed 1 year ago

lydiascarf commented 1 year ago

The old jekyll image isn't well maintained and we've been seeing a liquid error related to the config yaml.

netlify[bot] commented 1 year ago

Deploy Preview for jkan-demo ready!

Name Link
Latest commit 1710951eef83ceed16e6a54a2885817824964dfa
Latest deploy log https://app.netlify.com/sites/jkan-demo/deploys/640271e3f2a9920008ee27f5
Deploy Preview https://deploy-preview-254--jkan-demo.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

timwis commented 1 year ago

@BryanQuigley did you see my comment? Haven't tested, but pretty sure editing will be broken unless we fix that or add an installation documentation step to change that line in the config.

BryanQuigley commented 1 year ago

No, sorry that comment just links back to this page -but doesn't show anything else. The one you just made is your first comment I see here.

timwis commented 1 year ago

πŸ€” Strange. It was a comment on L5 of _config.yml:

I think the error you have been seeing relating to this was thrown by the github-metadata gem (pulled in by the github-pages gem). It normally checks git remote -v to infer the repository, but somehow it's not seeing that in the docker container (is .git in dockerignore perhaps?), so it requires the explicit declaration to fetch the metadata from GitHub.

The problem with explicitly declaring it here is that every new install would need to change it, otherwise the editor ui and 'edit on GitHub' links will point to the upstream repo. Plus it could be generated by the deploy scripts, so it would really only be necessary for the docker setup.

I'd recommend we try and fix how it infers the repo so we don't need the explicit declaration (if it's not an easy fix perhaps we do it in another PR to get this one through). And ultimately we should probably try and drop the gem entirely because it's been pretty annoying and I'm pretty sure it's effectively deprecated at this point.

I've just tested it now, and I can't reproduce the issue @lydiascarf was experiencing. Running docker compose up works fine for me with and without that repository: line in _config.yml. But if you build the site, you can see what I was referring to: that value is what gets written to /editor/index.html and used for all the dataset/org page "Edit on GitHub" links. So we'd need to include "change that line in _config.yml" in the installation instructions, or remove that line from _config.yml in this repo and try to reproduce the warning that @lydiascarf had so we can try and fix it with the docker config.

BryanQuigley commented 1 year ago

It was something we had to change for next.opendataphilly for it to work - I thought, will investigate. but I agree it would be nice to not have it.

I still get: Liquid Exception: No repo name found. Specify using PAGES_REPO_NWO environment variables, 'repository' in your configuration, or set up an 'origin' git remote pointing to your github.com repository. in /_layouts/dataset.html

BryanQuigley commented 1 year ago

Actually, maybe it's fine to require changes to _config? That's where you would put in things like Google Analytics/Twitter handles, etc

timwis commented 1 year ago

I still get: Liquid Exception: No repo name found. Specify using PAGES_REPO_NWO environment variables, 'repository' in your configuration, or set up an 'origin' git remote pointing to your github.com repository. in /_layouts/dataset.html

Yes that's the error I thought you might be gettingβ€”I got it when deploying to Netlify, and wrote this plugin to add it (and other values normally inferred by the github-metadata gem) at build time. Bizarre that you're getting it in docker πŸ€” the only thing I can think of is that somehow it can't read your .git directory from inside the container.... Would you mind just testing that? e.g. docker compose exec jekyll git remote -v

BryanQuigley commented 1 year ago

You are correct - and I still don't understand why it worked for you in docker. Needed to add it as a trusted directory and now it works without specifying the line in _config.

PR: #257