Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201611062153.uA6LrnT5000637@ip-10-146-233-104.ec2.internal>
Date: Sun, 6 Nov 2016 21:53:48 +0000
From: "David Knupp (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Henry Robinson <henry@cloudera.com>, Jim Apple <jbapple@cloudera.com>
Reply-To: dknupp@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D=28asf-site=29_Initial_commit_of_the_blog_section_of_the_Impala_ASF_website=2E=0A?=
In-Reply-To: <gerrit.1478230020000.Iaa578a70237fcc97589c667c17a70d3d6dad5ae1@gerrit.cloudera.org>
References: <gerrit.1478230020000.Iaa578a70237fcc97589c667c17a70d3d6dad5ae1@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Sun, 06 Nov 2016 21:53:54 -0000

David Knupp has posted comments on this change.

Change subject: Initial commit of the blog section of the Impala ASF website.
......................................................................


Patch Set 1:

(18 comments)

http://gerrit.cloudera.org:8080/#/c/4944/1//COMMIT_MSG
Commit Message:

PS1, Line 16: work properly
> "work properly if Pelican is to be used for non-blog pages -- a job"
Done


PS1, Line 25: comments section
> This is presumably not possible with a static site generator without disqus
Exactly. Disqus seems to be a popular approach.


http://gerrit.cloudera.org:8080/#/c/4944/1/Makefile
File Makefile:

Line 1: PY?=python
> This deserves comments.
To get this up quickly for people to start adding articles, my approach was to just do the bare bones, default creation of a Pelican site, and not invest too much time in tweaking or pruning it. I didn't want to hastily delete something, and if/when we decide to spend more time on the site later, have to then re-spawn or re-fetch a file that had been removed earlier.

By default, you get this Makefile and the publishconf.py file when you create a new site. We actually don't use ANY of it, so I haven't touched a single line. I'll go ahead and delete them.


http://gerrit.cloudera.org:8080/#/c/4944/1/blog/README.md
File blog/README.md:

Line 5: 
> extra blank line?
Done


PS1, Line 20:  path/to/venv
> What is this path? How do I find it?
Ah. It can be any path you want it to be. This is the canonical way that a typical python developer works with virtual environments. You use the virtualenv tool to create a new virtual environment some place that makes sense to you, which you can then activate and deactivate as needed. It's common to have multiple isolated environments for different projects, that may each have different versions of common libs, or even different versions of the python executable. 

E.g, if you run:

   $ source $IMPALA_HOME/infra/python/env/bin/activate
   (env) $

then anywhere you are, whenever you invoke plain ol' python, you're actually using the executable inside infra/python/env/bin/ that we're accustomed to invoking indirectly via the 'impala-python' wrapper.

   (env) $ which python
   /home/dknupp/Impala/infra/python/env/bin/python

I've updated the README to provide more information about using virtualenv.


PS1, Line 24: (impala_blog)
> What is this?
See updated README.


Line 32: Articles are stored as markdown files in the ```blog_content/``` directory, and follow a specific template. The name of the file is not important, as long as it's unique. You sould probably create this file on a new working branch in the repo.
> Long line, here and below
Done


Line 108: Don't forget to ```deactivate``` your virtualenv when you're done working on the blog.
> How?
See updated README.


http://gerrit.cloudera.org:8080/#/c/4944/1/pelicanconf.py
File pelicanconf.py:

Line 1: #!/usr/bin/env python
> licence header, please
I wasn't sure what to do about this. I noticed that while some of our third party code includes a license header:

https://github.com/apache/incubator-impala/blob/master/www/bootstrap/js/bootstrap.js

Some of our third party code doesn't:

https://github.com/apache/incubator-impala/blob/master/shell/ext-py/prettytable-0.7.1/setup.py

Can you tell me explicitly what to put here? (FYI, Pelican has the AGPL license: https://github.com/getpelican/pelican/blob/master/LICENSE.)


Line 3: from __future__ import unicode_literals
> Why?
This is code that many developers put at the beginning of their python files to ease transition from 2 to 3. Python 3 has several backward-breaking changes. The standard list of __future__ imports actually includes more. There's also absolute_import, division, and print_function. (Aside: if we ever have to support Python 3 in Impala, we may be somewhat screwed. For starters, I see a lot print statements liberally scattered throughout our code base that will all raise exceptions under python 3.x.)

I didn't actually put this line in this file. When the file was auto-spawned, it was there. All I did in this file was provide meaningful values for a few of the settings. For more specific info on this line, see http://blog.getpelican.com/pelicans-unified-codebase.html for more info.


Line 5: AUTHOR = u'Impala Dev'
> Does this need to be customized?
More than it is?  The docs said to make this setting the "default author." Blog posts will still respect the Author: field in markdown files. I think it only gets used of we don't specify a blog author, or if we explicitly use {{AUTHOR}} on one of the template files.


Line 22: # Blogroll
> These could be removed or more clearly explained
Boilerplate that came when this file was spawned. (A lot of blogs have a blogroll.) Removed.


Line 32: DEFAULT_PAGINATION = 100
> 100 posts per page?
Oops. I think that was supposed to be 10. How about 10?


Line 38: # Uncomment following line if you want document-relative URLs when developing
> Why not?
Links might get screwy with half the site being pelican managed, and the other half not, especially considering the fact that pages might be nested at different levels. For example, with the static nav bar in the main page template, what happens when some pages are at the root level, some might be in the blog/ directory, and still others might (at some point) be in the blog/categories/ directory? Absolute URLs worked out-of-the-box.

Also, the docs said this, so I left it at the default (which was commented out.)

"Defines whether Pelican should use document-relative URLs or not. Only set this to True when developing/testing and only if you fully understand the effect it can have on links/feeds."


http://gerrit.cloudera.org:8080/#/c/4944/1/themes/impala_asf_site_theme/templates/base.html
File themes/impala_asf_site_theme/templates/base.html:

Line 2: <html lang="{{ DEFAULT_LANG }}">
> It would be good if this file mentioned in an HTML comment at the top that 
Actually, this being a template file, it's exactly a file that gets hand-edited. It's from files like this (and the others in the template directory) that the actual site files -- index.html, my-blog-post.html, etc. -- are generated. In other words, let's say some day the Github address changes -- from "incubator-impala" to just "impala." This is the file where we'd edit the link in the nav bar. Then we'd run pelican, and the change would propagate to all of the individual page files that pelican manages.


http://gerrit.cloudera.org:8080/#/c/4944/1/themes/impala_asf_site_theme/templates/gosquared.html
File themes/impala_asf_site_theme/templates/gosquared.html:

Line 1: {% if GOSQUARED_SITENAME %}
> What is this all about?
We needed template files, and there's a "starter" theme that you can get with Pelican that you can modify.

https://github.com/getpelican/pelican/tree/master/pelican/themes/simple/templates

I only modified the templates we obviously needed, and left the others untouched -- assuming they might be useful later. (e.g., once we get enough articles, the pagination template will probably come into play.) Admittedly, I didn't look at each template file too closely, or test every contingency. However, this one in particular seems like it won't ever be used. I'll delete it.


http://gerrit.cloudera.org:8080/#/c/4944/1/themes/impala_asf_site_theme/templates/page.html
File themes/impala_asf_site_theme/templates/page.html:

Line 12: 		Last updated: {{ page.locale_modified }}
> We generally avoid tabs
Ah, good eye. Again, this is a standard file that came with the theme, and I didn't actually edit this one.


http://gerrit.cloudera.org:8080/#/c/4944/1/themes/impala_asf_site_theme/templates/tag.html
File themes/impala_asf_site_theme/templates/tag.html:

> What is this for?
Weird, right?

https://github.com/getpelican/pelican/blob/master/pelican/themes/simple/templates/tag.html


-- 
To view, visit http://gerrit.cloudera.org:8080/4944
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa578a70237fcc97589c667c17a70d3d6dad5ae1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple@cloudera.com>
Gerrit-HasComments: Yes