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

Create directory for Pulp Nginx snippets #278

Merged
merged 2 commits into from Apr 29, 2020
Merged

Create directory for Pulp Nginx snippets #278

merged 2 commits into from Apr 29, 2020

Conversation

fao89
Copy link
Member

@fao89 fao89 commented Apr 28, 2020

@fao89 fao89 force-pushed the 6594 branch 5 times, most recently from 18eb764 to 14aef5d Compare April 29, 2020 17:45
@fao89 fao89 force-pushed the 6594 branch 5 times, most recently from 14194c9 to 5d484bf Compare April 29, 2020 20:51
@fao89 fao89 requested a review from mikedep333 April 29, 2020 20:52
Copy link
Member

@mikedep333 mikedep333 left a comment

Choose a reason for hiding this comment

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

2 comments, but thank you for this 👍

Comment on lines 35 to 41
- name: Find pulp obsolete symlinks
find:
paths: /etc/nginx/conf.d
patterns: "pulp*"
register: old_pulp_symlinks
tags: cleanup
Copy link
Member

Choose a reason for hiding this comment

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

We should limit it to symlinks, in case users happen to put any custom files there:
https://docs.ansible.com/ansible/latest/modules/find_module.html#parameter-file_type

@@ -75,7 +75,7 @@ http {
proxy_pass http://pulp-api;
}

include conf.d/*.conf;
include pulp/*.conf;
Copy link
Member

Choose a reason for hiding this comment

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

The Debian family has a different /etc/ filesystem layout for nginx. Looking at their layout (in a container for example) and our config files, are you confident this will work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

@fao89 fao89 Apr 29, 2020

Choose a reason for hiding this comment

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

Debian seems to have the same path:
https://wiki.debian.org/Nginx/DirectoryStructure

Copy link
Member

Choose a reason for hiding this comment

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

I think you misunderstood my question.

I looked at the Fedora container's vs the debian container's folder structures. I see no reason why a /etc/nginx/pulp/ directory would not be recognized on Debian or interfere with Debian's utilities that maintain its nginx symlinks (which manages the contents of the "enabled" dirs, based on the contents of the "available" dirs), so it's safe to assume that it will function correctly. That's what I was asking.

See the file structure below for future reference:

root@debian-10:
/etc/nginx# ls -1p
conf.d/
fastcgi.conf
fastcgi_params
koi-utf
koi-win
mime.types
modules-available/
modules-enabled/
nginx.conf
proxy_params
scgi_params
sites-available/
sites-enabled/
snippets/
uwsgi_params
win-utf
[root@fedora-31 /etc/nginx]# ls -1p
conf.d/
default.d/
fastcgi.conf
fastcgi.conf.default
fastcgi_params
fastcgi_params.default
koi-utf
koi-win
mime.types
mime.types.default
nginx.conf
nginx.conf.default
scgi_params
scgi_params.default
uwsgi_params
uwsgi_params.default
win-utf

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I really misunderstood, now it is clear to me, thanks!

@mikedep333
Copy link
Member

Good job!

@fao89 fao89 mentioned this pull request Apr 29, 2020
@fao89 fao89 merged commit 503939e into pulp:master Apr 29, 2020
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.

None yet

2 participants