8000 Filter Nginx includes by sites present on target server by dalepgrant · Pull Request #1573 · roots/trellis · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fil 8000 ter Nginx includes by sites present on target server #1573

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
May 21, 2025
Merged

Filter Nginx includes by sites present on target server #1573

merged 6 commits into from
May 21, 2025

Conversation

dalepgrant
Copy link
Contributor
@dalepgrant dalepgrant commented May 1, 2025 •
edited
Loading

One for those of us using multiple remote servers with Trellis.

Currently, Trellis templates out anything you put into nginx-includes/ to the remote servers. Take the following:

trellis/
└── group_vars/         
    └── production/
        ├── main.yml
        └── vault.yml
    └── staging/
└── host_vars
    └── server1/
        └── wordpress_sites.yml
    └── server2/
        └── wordpress_sites.yml
    └── staging1/
    └── staging2/

Assume server1 has a different site(s) to server2, and perhaps staging1 and staging 2 have additional sites. Your Nginx includes folder might look like this:

trellis/
└── nginx-includes/         
    └── all/
        ├── rewrites.conf.j2
        └── proxy.conf.j2
    └── site1/
        └── rewrites.conf.j2
    └── site2/
        └── rewrites.conf.j2
    └── site3/
        └── rewrites.conf.j2
...etc

All servers will end up with all Nginx includes files, even if the sites don't exist on that server:

/etc/nginx/includes.d/
└── all/  
    ├── rewrites.conf
    └── proxy.conf
└── site1/  
    └── rewrites.conf
└── site2/  
    └── rewrites.conf
└── site3/  
    └── rewrites.conf
└── site4/  
    └── rewrites.conf

This PR filters the Nginx includes that are to be 'included' by combining the sites that exist on the target server (taken from wordpress_sites keys) with a default 'all'. This follows with the default setup shown in the docs.

Also included is the ability to override the default 'all' folder. I've only included this because it is possible to override the block in the Nginx conf template and if someone did that, they may also want to determine their own Nginx includes locations.

To override or otherwise add to the Nginx includes locations, in e.g. group_vars/all/main.yml you can do

# Edit: this change was removed

Note: in testing, the folders for sites not present on a remote are not cleaned up if they already exist but the files inside them are. That's to do with what happens when setting nginx_includes_d_cleanup: true which is a separate issue.

@dalepgrant dalepgrant marked this pull request as ready for review May 1, 2025 07:26
swalkinshaw
swalkinshaw reviewed May 17, 2025
View reviewed changes
@dalepgrant dalepgrant requested a review from swalkinshaw May 20, 2025 00:29
dalepgrant added a commit to dalepgrant/docs that referenced this pull request May 20, 2025
@dalepgrant
Add nginx includes docs for overriding default includes directories
57e96b9 
Rel: roots/trellis#1573
@dalepgrant dalepgrant mentioned this pull request May 20, 2025
Merged
@dalepgrant
Copy link
Contributor Author

Docs added: roots/docs#544

@swalkinshaw
Copy link
Member

Also included is the ability to override the default 'all' folder. I've only included this because it is possible to override the block in the Nginx conf template and if someone did that, they may also want to determine their own Nginx includes locations.

Is there really a point to this though? Even if someone customized that block, I'm not sure why they'd change the "all" folder. And I'm not sure I see a use case for having more than one when they do the same thing?

Personally I'd rather just omit this extra functionality since I don't see any use case.

@dalepgrant
Copy link
Contributor Author

I have to admit I didn't question the "why" someone might want to, only that they can elsewhere and so that should probably be catered for. Otherwise someone else in a few month's time might stumble across it and raise it as a bug for you guys to spend time reviewing & maybe fixing.

Doesn't affect me personally so I'm happy to take it out if you say so but given it's already there and keeps that functionality complete, I'd vote for it to stay in.

Let me know what you decide 👍

@swalkinshaw
Copy link
Member

Let's remove it and keep this simple ✨

@swalkinshaw swalkinshaw merged commit ed23739 into roots:master May 21, 2025
2 checks passed
@swalkinshaw
Copy link
Member

Thanks!

@dalepgrant
Copy link
Contributor Author

Ability to override the default 'all' folder has been removed (2d47410)

Tested the key motivation for this PR:

  • ✅ nginx-includes/example2.com/*.conf.j2 is NOT templated to /etc/nginx/includes.d/example2.com/*.conf (where example2.com is NOT present in wordpress_sites.yml)

Tested existing functionality hasn't been affected:

  • ✅ nginx-includes/all/*.conf.j2 is templated to /etc/nginx/includes.d/all/*.conf
  • ✅ nginx-includes/all/subdir/*.conf.j2 is templated to /etc/nginx/includes.d/all/subdir/*.conf
  • ✅ nginx-includes/example1.com/*.conf.j2 is templated to /etc/nginx/includes.d/example1.com/*.conf (where example1.com is the site key in wordpress_sites.yml)
  • ✅ Removing a file e.g. nginx-includes/example3.com/rewrites.conf.j2 removes that file from /etc/nginx/includes.d/example3.com/rewrites.conf. It does not remove empty folders.

@dalepgrant dalepgrant deleted the feature/prevent-nginx-includes-for-sites-not-present-on-server branch May 21, 2025 02:33
dalepgrant added a commit to dalepgrant/docs that referenced this pull request May 21, 2025
@dalepgrant
Update nginx includes docs
d4dfca3 
Rel: roots/trellis#1573
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swalkinshaw swalkinshaw Awaiting requested review from swalkinshaw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dalepgrant @swalkinshaw
0