Skip to content

Update sites.yml - #20

Closed
wgregorian wants to merge 1 commit into
ANXS:masterfrom
wgregorian:master
Closed

Update sites.yml#20
wgregorian wants to merge 1 commit into
ANXS:masterfrom
wgregorian:master

Conversation

@wgregorian

Copy link
Copy Markdown

It made sense to create the directories and enable the virtual sites for me. Wondering if you think it's a good idea?

@otakup0pe

Copy link
Copy Markdown
Member

These should probably both be opt-in options rather than defaults.

@pjan

pjan commented Dec 21, 2014

Copy link
Copy Markdown
Contributor

@otakup0pe I think that's exactly what this does? @wgregorian would you mind also updating the documentation/readme and defaults (should be empty hashes) to reflect this change in the PR?

@wgregorian

Copy link
Copy Markdown
Author

@pjan I was reviewing my changes from 5 months ago, I believe the defaults are already in place in README for this PR since it re-uses all existing vars. Have I missed something really obvious?

@soupdiver

Copy link
Copy Markdown
Contributor

I've added a similar change for myself but introduced a variable enabled_sites. Which links from sites-available to sites-enabled based on the name entry.
But you wouldn't have to delete the while site definition if you want to disable the site.
What you think about this?

@otakup0pe

Copy link
Copy Markdown
Member

I would also prefer the distinction between enabled and available sites to be represented by different variables.

@soupdiver

Copy link
Copy Markdown
Contributor

Ability do enable/disable sites was introduced with #27.
What do you think about creating the directories for the enabled sites?
In my opinion this should be handled by the deployment process of the site and not by this nginx role. Also the desired path could differ from {{nginx_www_dir}}/{{item.server.name}}
@pjan @otakup0pe

@farridav

Copy link
Copy Markdown

closing this in favour of #29

@farridav farridav closed this Feb 27, 2015
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.

5 participants