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

Use H5BP server configs #37

Merged
merged 1 commit into from
Oct 3, 2014
Merged

Conversation

austinpray
Copy link
Contributor

This has been tested locally but I would like to try to deploy it on digital ocean to see if it runs alright on a remote server (shouldn't be an issue)

Any feedback on this?


Checks out a stable version of h5bp/server-configs-nginx and applies it
to the /etc/nginx folder

http://goo.gl/pBfB4W

Misc:
Adds "restart nginx" handler

closes #31

@@ -0,0 +1,3 @@
---
- name: be sure nginx is restarted
action: service name=nginx state=restarted
Copy link
Member

Choose a reason for hiding this comment

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

Since this isn't used I'm not sure we should include it by default yet.

This should ideally be identical to https://github.com/roots/bedrock-ansible/blob/master/roles/wordpress-sites/handlers/main.yml if we kept it in. Annoying that Ansible can't call handlers from another role.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove it

@austinpray
Copy link
Contributor Author

How about now?

@nathanielks
Copy link
Contributor

Any votes against including h5bp/location/expires.conf as well? Will help with browser caching of static assets.

@nathanielks nathanielks mentioned this pull request Aug 29, 2014
@austinpray
Copy link
Contributor Author

If we enable caching wouldn't we need to implement some sort of revving process?

@nathanielks
Copy link
Contributor

Aye. It's built in to WP's wp_enqueue_anything, though it adds a query string to the end of the file with the version number, which this guy recommends against. Wufoo proposed another solution.

Either way, by default we have a revving process built into WordPress. If we want to be more "legit," we could also look into implementing another solution.

@austinpray
Copy link
Contributor Author

Provided we bake in the Wufoo solution by default I don't see a problem with it.

But enabling caching might have to be another issue separate from my basic integration PR here.

@nathanielks
Copy link
Contributor

Totally agree. That'd be a great separate PR

command: cp -r /tmp/server-configs-nginx/{{ item }} /etc/nginx/{{ item }}
with_items:
- 'mime.types'
- 'h5bp/'
Copy link
Member

Choose a reason for hiding this comment

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

Double quotes. Remember that you're the one who convinced me to switch :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

( ͡° ͜ʖ ͡°)

@swalkinshaw
Copy link
Member

Looks good other than the few minor formatting issues 👍

@austinpray
Copy link
Contributor Author

Fixed the formatting issues. Thanks for keeping my ass in line ;).

@austinpray
Copy link
Contributor Author

@swalkinshaw
Copy link
Member

I was going to comment about using copy then I looked up the docs, and yeah, only for local to remote. Linters are useful but only if they're correct!

@swalkinshaw
Copy link
Member

One more thing. Can we add this? https://github.com/h5bp/server-configs-nginx/blob/master/sites-available/no-default. Something like:

- name: Enable better default site to drop unknown requests
  file: src=/tmp/server-configs-nginx/sites-available/no-default
        dest="/etc/nginx/sites-enabled/no-default.conf"
        owner=root
        group=root
        state=link

We may want to copy that file to sites-available before linking.

@austinpray
Copy link
Contributor Author

Sure thing

@nathanielks
Copy link
Contributor

Old issue in this thread, but worth bringing up again: completely forgot the Roots theme already has filename-based revving built in, so there's that.

@austinpray
Copy link
Contributor Author

gonna add that thing @swalkinshaw requested tonight

@swalkinshaw
Copy link
Member

@austinpray ^ 🐱

@austinpray
Copy link
Contributor Author

Good to go.

@swalkinshaw
Copy link
Member

Looks like you did a rebase of some sort but can this be squashed into 1 commit as well?

Checks out a stable version of h5bp/server-configs-nginx and applies it
to the /etc/nginx folder

http://goo.gl/pBfB4W

Misc:
Adds "restart nginx" handler

Moved nginx variables to defaults yml

formatting issues

Enable better default site to drop unknown requests
@austinpray
Copy link
Contributor Author

welp. I think I borked it.

@austinpray
Copy link
Contributor Author

okay fixed lol

swalkinshaw added a commit that referenced this pull request Oct 3, 2014
@swalkinshaw swalkinshaw merged commit 9176489 into roots:master Oct 3, 2014
@austinpray austinpray deleted the H5BPserverconfigs branch October 4, 2014 15:43
@teohhanhui
Copy link
Contributor

Would be good to have the expires issue sorted out... Is there any downside when including it?

@swalkinshaw
Copy link
Member

@teohhanhui the downside is that you might not get the latest version of the files if you don't have cache busting in place. This comment still applies.

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.

Use H5BP server configs
4 participants