Spike out jekyll-remote-theme #1

benbalter · 2017-09-08T22:07:04Z

This is a first pass at arbitrary/remote theme support for Jekyll (and hopefully eventually GitHub Pages).

Usage

in _config.yml add

remote_theme: foo/bar

or

remote_theme: foo/bar@git_ref

What it does

~~Clones~~ Downloads and unzips the remote theme to ~~_theme~~ a temporary directory and sets it as site.theme

Gotchas

I'd love to be able to just use the theme key, but the way the Site object is created, it'd raise a "dependency not found error". We could theoretically monkey patch, but I thought remote_theme was a cleaner solution. ~~(some of the test are still intended to go the other way)~~

2. After the initial clone, _theme exists in the source dir. This is fine, until that _theme dir is committed and pushed, or the theme is update (meaning the two are out of sync. Potential workarounds: (1) warn if _theme isn't in .gitignore, (2) Do a fresh clone on each build (3) clone to a random temp dir outside of source.

/cc @parkr

parkr

This looks really good!! Nice work 🎉

parkr · 2017-09-09T13:18:56Z

lib/jekyll-remote-theme/cloner.rb

+          return false
+        end
+
+        Jekyll.logger.info "Remote theme: ", "Cloning into #{git_url}"


Cloning into path, right?

Ooph. Verified locally and this is currently the opposite of how Git is implemented (and users would expect). Will update.

parkr · 2017-09-09T13:21:51Z

lib/jekyll-remote-theme/cloner.rb

+
+      private
+
+      def clone_command


In safe mode, we'll want to disable any credentials access and restrict to the https protocol only.

It's restricted to HTTPS because the use can only pass the name + owner + ref. We lock it to https://github.com/*

benbalter · 2017-10-19T21:41:04Z

@pathawks, @DirtyF by no means expected, but I respect your 👀, if you're interested in this functionality (potentially for GitHub Pages), and have a moment to review, I'd ❤️ any feedback you could provide on the implementation.

benbalter · 2017-10-19T21:44:16Z

Some additional context...

Current implementation

After several iterations and discussions with @parkr the process is as follows:

The plugin curls the codeload version of the user-supplied repo (+ git ref) to a temporary directory
We unzip the theme to a temporary directory
We set a custom Jekyll::RemoteTheme::Theme as Jekyll's site.theme, with the root in the temporary directory
We build the site as if it were a Gem-based theme (but no support for Ruby dependencies)

Other considerations

The first implementations stored the site in a _themes folder (where it would likely be committed to the repo when downloaded locally) and used Git to clone the repo (which uses the local users creds, etc.).

Since we don't need the history, and don't want the checkout to live where it can be committed, we opted for curl + unzip in a temp dir that gets destroyed after build.

pathawks · 2017-10-19T22:03:28Z

This is awesome! 😮:+1:

parkr

Very, very nice! Download, extract, wire up. Cleanly done. Just a few notes for some nits (including some suggestions for a later iteration around optimizing for bad shelling-out environments) but no blockers near as I can tell.

One thing I might recommend is a simple smoke test. You have excellent unit tests. It'd be great to have a simple script and a fixture jekyll site that just runs jekyll build -s fixture_site -d output and checks that the theme is properly loaded etc. The actual Jekyll integration isn't well-tested unless my 10PM head cold/flu brain is making my memory fuzzy.

Anyway, excellent work. ✨

parkr · 2017-10-20T01:30:41Z

jekyll-remote-theme.gemspec

+  s.summary       = "Jekyll plugin for building Jekyll sites with any GitHub-hosted theme"
+
+  s.files         = `git ls-files app lib`.split("\n")
+  s.platform      = Gem::Platform::RUBY


Is platform required here? This might limit things...

/shrug. Was part of the boilerplate. Removed via fa461f2.

parkr · 2017-10-20T01:37:23Z

lib/jekyll-remote-theme.rb

@@ -0,0 +1,27 @@
+require "jekyll"


Can you add the magic # frozen_string_literal: true comment to the top of all your Ruby files? I'm surprised Jekyll 3.6's rubocop.yml file didn't include that...

Once that's done, you can get rid of the .freeze calls.

Believe jekyll/jekyll#6444 was the reason it wasn't added. Added via 8cc5936.

parkr · 2017-10-20T01:37:38Z

lib/jekyll-remote-theme.rb

+    autoload :VERSION,     "jekyll-remote-theme/version"
+
+    CONFIG_KEY = "remote_theme".freeze
+    LOG_KEY    = "Remmote Theme:".freeze


parkr · 2017-10-20T01:38:15Z

lib/jekyll-remote-theme/downloader.rb

+    class Downloader
+      include RemoteTheme::Executor
+
+      HOST = "https://codeload.github.com".freeze


Would it be useful to allow us to override these with environment variables? Maybe a later version.

parkr · 2017-10-20T01:41:45Z

lib/jekyll-remote-theme/downloader.rb

+          return
+        end
+
+        download


If this fails, how will a user be told?

Right now, if Executor gets a non-zero exit code, it raises a DownloadError and dumps stdout.

parkr · 2017-10-20T01:59:37Z

spec/jekyll-remote-theme/downloader_spec.rb

+    end
+
+    it "extracts the theme" do
+      expect("#{theme.root}/_layouts/default.html").to be_an_existing_file


parkr · 2017-10-20T02:00:55Z

spec/jekyll-remote-theme/munger_spec.rb

+
+  context "without a theme" do
+    before { site.config["theme"] = nil }
+    before { subject.munge! }


Can these be part of the same before block instead of having 2?

parkr · 2017-10-20T02:01:27Z

spec/jekyll-remote-theme/munger_spec.rb

+  end
+
+  context "without a theme" do
+    before { site.config["theme"] = nil }


Or maybe this could be done via a let(:config)?

parkr · 2017-10-20T02:03:07Z

spec/jekyll-remote-theme/theme_spec.rb

+RSpec.describe Jekyll::RemoteTheme::Theme do
+  let(:owner) { "foo" }
+  let(:name) { "bar" }
+  let(:nwo) { "foo/bar" }


let(:nwo) { "#{owner}/#{name}" }?

parkr · 2017-10-20T02:03:45Z

spec/jekyll-remote-theme/theme_spec.rb

+    let(:nwo) { " FoO/bAr " }
+
+    it "normalizes the nwo" do
+      expect(subject.instance_variable_get("@raw_theme")).to eql("foo/bar")


Do we know for sure that codeload will respect these kinds of capitalization differences? E.g. my theme is BenBalter/foOOOO but we request benbalter/fooooo.

Yes. I tested and it's case insensitive.

DirtyF · 2017-10-20T09:40:00Z

This is so cool. Users gonna love this. Well done @benbalter

One question though, how are theme dependencies handled? 📦📦📦📦

The typical use case I imagine for this (correct me if I'm wrong), is that one might want to try a few different themes without having to fork a repo or install anything. You just change the value for the remote_theme in your config. And this is what I've done with the default jekyll new website. I removed theme: minima to use remote_theme: benbalter/retlab instead.

One of the dependencies isn't met so here is what I got:

 Remmote Theme: Using theme benbalter/retlab
  Liquid Exception: Liquid syntax error (/private/var/folders/4s/1tjg18y173934vzsz9lg3mf80000gn/T/jekyll-remote-theme-20171020-80204-1l6htmc/retlab-master/_includes/improve.html line 7): Unknown tag 'github_edit_link' included in /_layouts/page.html
jekyll 3.6.0 | Error:  Liquid syntax error (/private/var/folders/4s/1tjg18y173934vzsz9lg3mf80000gn/T/jekyll-remote-theme-20171020-80204-1l6htmc/retlab-master/_includes/improve.html line 7): Unknown tag 'github_edit_link' included

Hence my question: if jekyll-remote-theme is a way to use external themes on GitHub Pages how do you deal with theme dependencies?

Maybe this does not qualify for a 0.1 version but it's a common issue users will face rapidly.

For Jekyll users that don't host their site on GitHub Pages, should jekyll-remote handle this for the user or at least warn him that the theme has dependencies?

If you ask me, an optimal experience would be:

Using remote theme: benbalter/retlab
Installing theme dependencies…

benbalter · 2017-10-20T15:42:46Z

how are theme dependencies handled?

@DirtyF great question. Right now, they're not. Since we're allowing any user-supplied theme, we have to treat the theme as untrusted, arbitrary user input. In order to determine dependencies, we'd have to parse/eval the Gemspec, which could lead to arbitrary code execution.

In a subsequent version, we could theoretically use e.g., regex to determine dependencies, and auto require any whitelisted dependency. We likely won't be able to support arbitrary plugins given our current setup.

Edit: For now, you'll have to add the dependencies to the config file.

parkr

🎉

parkr · 2017-10-20T15:46:42Z

lib/jekyll-remote-theme/downloader.rb

+          *timeout_command, "curl", "--url", zip_url, "--output", zip_file.path,
+          "--user-agent", USER_AGENT, "--fail", "--silent", "--show-error",
+        ]
+        run_command(*cmd)


Maybe for a later iteration, but retries here might be nice. A network blip could cause downloads to fail so giving this a rescue ExecutionError with some recursion might be helpful to prevent these kinds of blips.

def download(tries: 3) run_command(*cmd) rescue ExecutionError => e raise e if tries <= 0 download(tries: tries - 1) end

❓

Agree, but hoping to follow up in a subsequent pass with #2 and #3 making this unnecessary.

benbalter · 2017-10-20T18:14:29Z

🤘 Thanks for all the feedback. Going to merge this initial spike, get out an initial release, and start work on #2 and #3.

initial commit

b3e0822

parkr reviewed Sep 9, 2017

View reviewed changes

benbalter added 12 commits September 11, 2017 12:49

update debug output

573b004

make clone operation more robust

8e34a52

properly stub theme git_url in tets

688ca72

clone to temp dir, not to repo

085ddbf

add clone_dir_exists? back

fc18254

cleanup when done

6bb8ac3

fix quotes

9745e3c

remove support for gem-based themes

78ff052

curl codeload zip file rather than cloning

46b1838

add downloader tests

0905fb9

add theme tests

45b6631

add docs

c62906c

parkr approved these changes Oct 20, 2017

View reviewed changes

benbalter added 3 commits October 20, 2017 09:37

remove platform from gemspec

fa461f2

frozen string literal all the things

8cc5936

s/remmote/remote/ in LOG_KEY

5f6bbf5

This was referenced Oct 20, 2017

Drop use of cURL in favor of pure-ruby implementation #2

Closed

Drop use of unzip in favor of pure Ruby implementation #3

Closed

benbalter added 6 commits October 20, 2017 09:48

use Addressable::URI to build zip URL

b6af041

memoize Executor#timeout_command

9ba3c9e

use File.file? not !File.dir?

8e01da7

git ref can be a branch, tag, or commit

11b82fc

remove Theme#invalid

0be09b1

add git_ref and root to Theme#inspect

5223bd2

benbalter added 7 commits October 20, 2017 10:44

document that we default to master

711ed4a

cleanup temp dir if it exists

e0d6ad8

use let to set config

ade85f7

use owner and name to set nwo

62e9f7a

add integration spec

a4b8153

add travis.yml

b2720d6

explicitly test a case sensitive theme

e933269

benbalter mentioned this pull request Oct 20, 2017

Use regex to parse Gemspec and require any whitelisted dependencies #4

Closed

parkr reviewed Oct 20, 2017

View reviewed changes

fix failing test

0a606db

benbalter merged commit 4e20221 into master Oct 20, 2017

benbalter deleted the spike branch October 20, 2017 18:14

daviddarnes mentioned this pull request Oct 22, 2017

Testing feedback #8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spike out jekyll-remote-theme #1

Spike out jekyll-remote-theme #1

benbalter commented Sep 8, 2017 •

edited

parkr left a comment

parkr Sep 9, 2017

benbalter Sep 11, 2017

parkr Sep 9, 2017

benbalter Sep 11, 2017

benbalter commented Oct 19, 2017

benbalter commented Oct 19, 2017

pathawks commented Oct 19, 2017

parkr left a comment

parkr Oct 20, 2017

benbalter Oct 20, 2017

parkr Oct 20, 2017

benbalter Oct 20, 2017

parkr Oct 20, 2017

parkr Oct 20, 2017

parkr Oct 20, 2017

benbalter Oct 20, 2017

parkr Oct 20, 2017

parkr Oct 20, 2017

parkr Oct 20, 2017

parkr Oct 20, 2017

parkr Oct 20, 2017

benbalter Oct 20, 2017

DirtyF commented Oct 20, 2017 •

edited

benbalter commented Oct 20, 2017 •

edited

parkr left a comment

parkr Oct 20, 2017

benbalter Oct 20, 2017

benbalter commented Oct 20, 2017

Spike out jekyll-remote-theme #1

Spike out jekyll-remote-theme #1

Conversation

benbalter commented Sep 8, 2017 • edited

Usage

What it does

Gotchas

parkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benbalter commented Oct 19, 2017

benbalter commented Oct 19, 2017

Current implementation

Other considerations

pathawks commented Oct 19, 2017

parkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirtyF commented Oct 20, 2017 • edited

benbalter commented Oct 20, 2017 • edited

parkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benbalter commented Oct 20, 2017

benbalter commented Sep 8, 2017 •

edited

DirtyF commented Oct 20, 2017 •

edited

benbalter commented Oct 20, 2017 •

edited