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

Fix docker spin up steps (#2) #254

Merged

Conversation

lydiascarf
Copy link
Collaborator

@lydiascarf lydiascarf commented Mar 3, 2023

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

  • repository added to config
  • moved from Jekyll image to Dockerfile based on Ruby image

@netlify
Copy link

netlify bot commented Mar 3, 2023

Deploy Preview for jkan-demo ready!

Name Link
🔨 Latest commit 1710951
🔍 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.

@lydiascarf lydiascarf force-pushed the feature/lydiascarf/2-fix-spinup-steps branch from a47b3e7 to a4361bf Compare March 3, 2023 21:22
@lydiascarf lydiascarf mentioned this pull request Mar 3, 2023
- repository added to config
- moved from Jekyll image to Dockerfile based on Ruby image
Copy link
Collaborator

@BryanQuigley BryanQuigley left a comment

Choose a reason for hiding this comment

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

Tested and fixed the docker issues I was facing + makes it easier to keep in line with other build processes.

@timwis
Copy link
Owner

timwis commented Mar 13, 2023

@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
Copy link
Collaborator

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
Copy link
Owner

timwis commented Mar 13, 2023

🤔 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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Owner

timwis commented Mar 14, 2023

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
Copy link
Collaborator

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

BryanQuigley pushed a commit that referenced this pull request Mar 14, 2023
Also remove unneeded lines from docker-compose.

Related: #254
BryanQuigley pushed a commit that referenced this pull request Mar 14, 2023
Also remove unneeded lines from docker-compose.

Related: #254
BryanQuigley referenced this pull request in opendataphilly/opendataphilly-jkan Mar 17, 2023
Also remove unneeded lines from docker-compose.

Related: #254
lydiascarf pushed a commit to lydiascarf/jkan that referenced this pull request Mar 21, 2023
Also remove unneeded lines from docker-compose.

Related: timwis#254
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