Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Pipeline css #1245

Merged
merged 29 commits into from
Aug 6, 2013
Merged

Pipeline css #1245

merged 29 commits into from
Aug 6, 2013

Conversation

chadwhitacre
Copy link
Contributor

This builds gittip.css on the fly from SCSS sources when it's requested rather than having to call:

watch -n1 make css

or

sass --watch scss/gittip.scss:www/assets/%version/gittip.css

Instead of depending on `make css` to rebuild css, this calls scss
dynamically whenever gittip.css is requested.
This lets us use the production file in local development, parallel to
how we use a cloud db at first.
Thought I had done this. Oh, well.
@chadwhitacre
Copy link
Contributor Author

Next step is to either:

@bruceadams
Copy link
Contributor

Does this perform acceptably well? For me, I'll want to run it (which I'm not going to manage today) before merging this. Others are welcome to merge as they see fit.

@chadwhitacre
Copy link
Contributor Author

@bruceadams It performs acceptably well in development. Sass does its own caching (one of the reasons I decided to go with the original sass tool instead of a libsass-based alternative). I haven't benchmarked it, however, so I'm not sure how it will look in production. Long-term we'll end up with a CDN, anyway.

@chadwhitacre
Copy link
Contributor Author

To be clear: this is not ready to merge yet. We're not ready to deploy this without further work as mentioned above (buildpack or CDN). Therefore merging this to master right now would be a bad idea. :-)

This was referenced Jul 29, 2013
@chadwhitacre
Copy link
Contributor Author

Seems that the way forward actually involves both installing sass at Heroku and using a CDN. I've set up a CDN w/ MaxCDN (closing #866), which uses https://www.gittip.com/assets/ as it's origin server. Here's an example of an asset served from this new CDN:

https://assets-gittipllc.netdna-ssl.com/9.4.7/gittip.css

Now we need to dynamically generate that file from the origin server and we're all set, and that means installing sass at Heroku.

I've ticketed writing a custom buildpack as #1257 and will circle back here after that lands.

This hook is part of heroku-buildpack-python.

ht @btubbs and @kennethreitz
I changed the gittip.css simplate to 404 if the version requested doesn't
match the current version of the app, so that we don't accidentally
serve CSS that's out of sync with our markup. Without this we'd actually
have a potential attack vector, in that someone could prefill our CDN
cache with the current version of the CSS by requesting future versions
from the CDN that would be served with the current version by the origin
server.

For development we don't want to require sass out of the box, and so we
want to be able to point to a CDN-hosted CSS file by default. Since the
version in development won't match the version in production, I added an
out: if you specify '-' as the version you won't get a 404, you'll get
the file. This changes default local.env to take advantage of that.
@chadwhitacre
Copy link
Contributor Author

@bruceadams @clone1018 I'm ready to review and merge this when one of yinz gets a chance. I ended up installing sass at Heroku using a post_compile hook provided by the Python buildback. We can now generate the CSS dynamically at Heroku:

https://gittip.whit537.org/assets/-/gittip.css

We've got a CDN set up that uses https://www.gittip.com/assets/ as its origin. We can use the new CSS_HREF environment variable to load CSS from the CDN, which will fall back to the origin on a cache miss:

https://assets-gittipllc.netdna-ssl.com/-/gittip.css

We had a cache_static module but I turned it off because it was
misbehaving: https://botbot.me/freenode/gittip/msg/4196280/. I'm
bringing these hooks back because it looks like we need to configure
cache headers to make it work with MaxCDN. There are facilities in the
MaxCDN dashboad for working around various issues, but they don't appear
to be working for me, and I'm getting less than stellar response from
the on-page live chat over there. So this is me trying to change the
headers at the origin so we start seeing cache hits at MaxCDN. I've set
up a pull zone for QA at MaxCDN so we can fully test this front to back
before deploying to production.
What if someone has a username assets-oh-yeah-assets?
@chadwhitacre
Copy link
Contributor Author

Okay @bruceadams @buttscicles @trinary @clone1018 @wyze @twolfson this is ready for some pounding:

https://gittip.whit537.org/

I brought back the gittip/cache_static.py hooks because we needed to set caching headers at the origin in order for the MaxCDN cache to function properly. The site now uses this CSS file:

https://assets-qa-gittipllc.netdna-ssl.com/9.4.7-dev/gittip.css

That's at MaxCDN with this as the origin:

https://gittip.whit537.org/assets/9.4.7-dev/gittip.css

I'm seeing X-Cache: HIT for the CDN version which is what we want:

$ curl -I https://assets-qa-gittipllc.netdna-ssl.com/9.4.7-dev/gittip.css
HTTP/1.1 200 OK
Date: Tue, 30 Jul 2013 13:48:08 GMT
Content-Type: text/css; charset=UTF-8
Connection: keep-alive
Cache-Control: public
Expires: Sun, 17 Jan 2038 19:14:07 GMT
Vary: Cookie
X-Gittip-Version: 9.4.7-dev
Server: NetDNA-cache/2.2
X-Cache: HIT

$

@clone1018 Let's make sure we don't see the flakiness you were seeing before. I believe that was caused by reset.css not being loaded properly; that file is in /assets/ but not in /assets/%version/.

We should particularly pay attention to how this interacts with gittip/authentication.py.


response.headers.cookie.clear()

if 'version' in uri.path:
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this cache indefinitely in development too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good catch. I'd prefer not to start switching features on the version string, but rather to maintain finer-grained configuration by adding more environment variable knobs. I've added a GITTIP_CACHE_STATIC knob which is set to no in default_local.env.

@joealcorn
Copy link
Contributor

Wouldn't it be better to compile and upload the css to the CDN as part of the deploy script?

Conflicts:
	www/assets/%version/gittip.css
@chadwhitacre
Copy link
Contributor Author

Wouldn't it be better to compile and upload the css to the CDN as part of the deploy script?

Perhaps. MaxCDN has two "zone" types: push and pull. They steer towards pull zones for assets, and push zones for large binary files. Of course there's no technical reason (that I'm aware of) we couldn't use a push zone for assets.

By generating CSS dynamically on the server, we introduce sass as an additional runtime dependency, which makes our slugs heavier and increases our surface area for potential vulnerabilities. We also add complexity to the build process because we have to install sass (I did this in a post_compile hook).

If we generate CSS dynamically at release time and push to our CDN, then we add release-time complexity. We can fail to upload the assets (network issues, API issues, etc.) and we would have to code defensively against that in release.sh. (I used the push method at YouGov for DataDirect, so I do have real-world experience with that method. )

Note that gittip.css is just the beginning. I'd like to do something similar for gittip.js, and then I'd like to introduce a second tier for CSS and JS dependencies, which change less frequently but should still be bundled together into a second asset. I'm thinking we should have just two CSS assets and two JS assets: one each for CSS and JS for our dependencies and our own code. The one for our dependencies would change infrequently, the one for our own code would change frequently. After that's tight we should start looking at spriting our images.

The fact is that we can't escape the run-time complexity, because we need to provide a good developer experience, and quickly compiling assets on the fly is the ideal way to do that. The good news is that this complexity gets exercised often by many developers. Exercise keeps a system healthy. If we introduce similar complexity at release time then it a) introduces duplication, and b) only gets exercised at release time, and then only by approximately one person.

I think we want to go with pull.

Conflicts:
	configure-aspen.py
@wyze
Copy link
Contributor

wyze commented Aug 3, 2013

Intense testing session here: https://botbot.me/freenode/gittip/msg/4960178/

@wimleers
Copy link

wimleers commented Aug 5, 2013

You want to go with pull. It's easier. It's handier.

You might find this useful for more context: http://wimleers.com/article/key-properties-of-a-cdn.

wyze added a commit that referenced this pull request Aug 6, 2013
@wyze wyze merged commit 96d8502 into master Aug 6, 2013
@chadwhitacre chadwhitacre deleted the pipeline-css branch August 6, 2013 05:11
@chadwhitacre chadwhitacre mentioned this pull request Aug 19, 2013
This was referenced Sep 16, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants