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

site.safe isn't the right way to set "production" mode #84

Closed
tommyblue opened this issue May 5, 2012 · 16 comments
Closed

site.safe isn't the right way to set "production" mode #84

tommyblue opened this issue May 5, 2012 · 16 comments
Assignees
Milestone

Comments

@tommyblue
Copy link
Contributor

I think that using site.safe as flag for production mode isn't the right way to do it.

This only works on Github because it uses --safe when the site is generated. I think that the real purpose of --safe used by Github is a matter of security. This can be false outside it, e.g. on my own server, Heroku or wherever I'm deploying my site and if I want to use a custom plugin (disabled by --safe).

This generates big confusion when migrating from/to github or when developing a generic theme, I think you should find a better way (custom option?) to define a production mode

@plusjade
Copy link
Owner

plusjade commented May 5, 2012

@tommyblue you are right about every point. --safe as an environment flag is a hack. Most of JB's features are hacks.
The problem is this no native way to set set environment flags automatically that work with GitHub Pages. This is the major point here. If we define a configuration variable via _config.yml then we are forcing the user to have to consciously remember to toggle to 'production' mode when pushing to GitHub, then remember to reset it to development when working locally. Manual user input is always a subpar user experience.

The entire reason why I'm piggybacking off of --safe is because it solves the above problem. It is the only way we can infer that Jekyll is being run by GitHub without any user input. For reference here is what GitHub says they override:

safe: true
source: <your pages repo>
destination: <the build dir>
lsi: false
pygments: true

I do understand the main issue: For users that intend to run custom plugins and deploy to their own setup, we get a collision of usage between disabling/enabling --safe.

However one of Jekyll Bootstrap's core design philosophies is to be 100% compatible with GitHub Pages, for superior convenience. With that in mind, plugins have to take a backseat to GitHub Pages support, quite simply because GitHub does not support any plugins.

I will always be open to suggestions for solving this issue though, as long as it remains compatible with GitHub pages.
What do you think?

P.S. The GitHub pages limitation IS a limitation. But that's what JB is built for, that's JB's goal. However not being able to control the development of Jekyll is why I created http://ruhoh.com - it's still in beta but will solve all of the problems we've run into with Jekyll, GitHub Pages, and static blogging. (supports automatic environment variable toggling, like rails, natively).

EDIT: seeing as how GitHub overrides the source directory:

source: <your pages repo>

I might be able to do some regex or something (as permitted by Liquid -_-) to infer the GitHub change. But I'll have to do some due diligence to see that it works in all cases.

@tommyblue
Copy link
Contributor Author

What about some config like:

JB:
  deploy: github

with some other possible values (heroku, private, etc) to set which kind of deploy will be used?

JB themes should check this value and not site.safe, so user would be free to use custom plugins.

@plusjade
Copy link
Owner

plusjade commented May 5, 2012

wont this still have to be set manually? environment flag is used to
dynamically set path variables. for example localhost should always have
root urls for simplicity. however when pushing to github your path may
change based on whether its pushed to a master branch or namespaced in a
repo using gh-pages branch. this produces urls that are namespaced by the
name of the repo.

so these things should ideally happen automagically based on environment.

@plusjade
Copy link
Owner

plusjade commented May 5, 2012

sorry im on mobile. I get what you mean now. specifying a deploy strategy
would determine how env works...ill think about this. sounds like it could
work!

@tommyblue
Copy link
Contributor Author

Thanks, I'll wait for your decision :)

@nolith
Copy link
Contributor

nolith commented May 6, 2012

I've got the same problem, but I wasn't aware of the custom plugins.
In my setup, without any custom plugin, I wrote a custom rake script like this

desc "Generate statically the site"
task :generate do
  require 'jekyll'
  opt = Jekyll.configuration({})
  source = opt['source']
  destination = opt['destination']
  site = Jekyll::Site.new(opt.merge({'safe' => 'true'}))
  puts "Building site: #{source} -> #{destination}"
  begin
    site.process
  rescue Jekyll::FatalException => e
    puts
    puts "ERROR: YOUR SITE COULD NOT BE BUILT:"
    puts "------------------------------------"
    puts e.message
    exit(1)
  end
  puts "Successfully generated site: #{source} -> #{destination}"
end

As you can see I'm changing safe only for deployment generation.

If you can add a deployment destination, like @tommyblue suggested, and add
this task into JB replacing safe with something like production; then you can get the current situation checking

(safe && github) || production

In order to avoid any issue to github page users you can default the deployment destination to github

@ubershmekel
Copy link

I wanted to use the jekyll-tagging plugin so I can have rss feeds for each tag. Problem was that I need BASE_PATH to work even when safe is false. So right now I can either have plugins or I can have BASE_PATH. A strange predicament IIUC.

Edit - my current solution is to edit manually _includes/JB/setup from this

{% if site.safe and site.JB.BASE_PATH and site.JB.BASE_PATH != '' %}

to this

{% if site.JB.BASE_PATH and site.JB.BASE_PATH != '' %}

@bettse
Copy link

bettse commented Apr 29, 2013

I really like @tommyblue's suggestion; I have run into this issue each time I upgrade jekyll-bootstrap on my personal (non-github) account. Each time I have to modify the jekyll-bootstrap code to compensate, which mades me sad.

@ghost ghost assigned plusjade Apr 29, 2013
@Twixer
Copy link

Twixer commented May 7, 2013

I use the same solution as @ubershmekel but finally it's not the best one as it implies to modify the core files of JB ...

@groundh0g
Copy link
Collaborator

According to https://help.github.com/articles/using-jekyll-with-pages/, GitHub Pages injects a "github:" metadata blob in the YAML. I'll run a few tests, but if that's true, looking for a non-empty "site.github" value might be a viable means of detecting production mode for GitHub Pages.

@groundh0g groundh0g assigned groundh0g and unassigned plusjade Mar 15, 2015
@groundh0g groundh0g added this to the v 0.4.0 milestone Mar 15, 2015
groundh0g added a commit that referenced this issue Mar 15, 2015
See comments in issue #84 for proposed strategy.
@groundh0g
Copy link
Collaborator

I just committed a solution at groundh0g@8fe0db8.

I still need to run some tests before I issue a pull request, but I wanted to open the floor to comments / concerns.

Note that GitHub Pages is given priority. The generic Jekyll environment check may indicate a production build, but the GitHub Pages check immediately follows, overriding the generic.

AleksueiR added a commit to RAMP-PCAR/ramp-pcar-docs that referenced this issue Mar 24, 2015
You can't build using plugins AND BASE_PATH that is not ""
plusjade/jekyll-bootstrap#84 (comment)
@groundh0g
Copy link
Collaborator

Added note (line-item comment), based on @AleksueiR's commit, to look at tweaking how JB/setup works.

groundh0g@8fe0db8#commitcomment-10424717

@groundh0g
Copy link
Collaborator

This is officially the last issue in v0.4.0 milestone. If you have comments on this, or any of the other issues in v0.4.0, please feel free to speak up. (Preferably in the issue that concerns you.)

https://github.com/plusjade/jekyll-bootstrap/issues?utf8=%E2%9C%93&q=milestone%3A%22v+0.4.0%22+

@groundh0g
Copy link
Collaborator

I'll commit this tonight and label master.

Missed due date by one day. D'Oh! =)

@groundh0g
Copy link
Collaborator

I got distracted by a shiny object - continuous integration with Jenkins in a Docker container. Long story, but promising stuff there.

Good to take the extra time, though. On further reflection, I don't think @AleksueiR's patch will be needed in the new logic. Plugins should work just fine, and sharing should still be disabled unless "production" mode is detected.

@groundh0g
Copy link
Collaborator

After a false start, this appears to be merged now.

asfgit pushed a commit to apache/zeppelin that referenced this issue Sep 7, 2016
### What is this PR for?
In recent changes download links were broken on the project website

### What type of PR is it?
Hot Fix

### Todos
* [x] - update Jekyll version
* [x] - update documentation, with `--safe` removed
* [x] - add "production" mode flag though `JEKYLL_ENV` aka plusjade/jekyll-bootstrap#84

### How should this be tested?
```
bundle update
JEKYLL_ENV=production bundle exec jekyll serve
```

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? Yes, it was

Author: Alexander Bezzubov <bzz@apache.org>
Author: Damien CORNEAU <corneadoug@gmail.com>
Author: Alexander <abezzubov@nflabs.com>

Closes #1410 from bzz/fix-download-links and squashes the following commits:

2b04e4a [Alexander] Merge pull request #11 from corneadoug/fix/otherRenderingIssues
f774bb0 [Damien CORNEAU] Add extension to pages_list links
68f4d64 [Damien CORNEAU] Backport plusjade/jekyll-bootstrap#293
09f770e [Alexander Bezzubov] Add missing dependency and update deprecated config
f270a43 [Alexander Bezzubov] Update Jekyll in order to use JEKYLL_ENV
45bf884 [Alexander Bezzubov] Backport of plusjade/jekyll-bootstrap#262 for website
aterai pushed a commit to aterai/jekyll-bootstrap that referenced this issue Mar 11, 2019
See comments in issue plusjade#84 for proposed strategy.
aterai pushed a commit to aterai/jekyll-bootstrap that referenced this issue Mar 11, 2019
aterai pushed a commit to aterai/jekyll-bootstrap that referenced this issue Mar 11, 2019
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

No branches or pull requests

7 participants