Skip to content

[ansible/artifactory] refactored Nginx role (DRY). #443

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

bbaassssiiee
Copy link
Contributor

PR Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • Title of the PR starts with installer/product name (e.g. [ansible/artifactory])
  • CHANGELOG.md updated
  • Variables and other changes are documented in the README.md

What this PR does / why we need it:
There were two roles in master that basically did the same thing and duplicated a lot of code:

  • artifactory_nginx
  • artifactory_nginx_ssl

I rewrote artifactory_nginx so it behaves just like the other two (which were mutually exclusive in use). Also I added a way to install the Nginx module 1.20 (or newer) from the Yum repo's configured on the machine. This is helpful in on-premises deployments where every proxy whitelist item needs permission from security.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
Partial fix for #323

Special notes for your reviewer:
Succeeds PR #337

@chukka chukka self-requested a review March 26, 2025 04:53
Copy link
Collaborator

@chukka chukka left a comment

Choose a reason for hiding this comment

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

Can you rebase from master again and see my comments .

Ideally, I 'm looking at single role for nginx and also , an option to choose (or skip ) upstream repos for nginx and postgresql

Do you want to update changes for postgresql role as well in the same PR - will be happy to review them .

@bbaassssiiee
Copy link
Contributor Author

bbaassssiiee commented Mar 26, 2025

I rather have this merged now, this PR is quite large and postgres changes would deserve a distinct PR.

@bbaassssiiee
Copy link
Contributor Author

bbaassssiiee commented Mar 27, 2025

Can you rebase from master again and see my comments .

Rebased and updated.

Ideally, I 'm looking at single role for nginx and also , an option to choose (or skip ) upstream repos for nginx and postgresql

nginx_upstream: true takes Nginx from nginx.org, false takes it from the distro.

Since PostgreSQL port would be firewalled anyway it makes sense to only use the distro, because of enterprise users.

Do you want to update changes for postgresql role as well in the same PR - will be happy to review them .

Have a look at this role which could be a more secure replacement than the one in this collection:

https://github.com/bbaassssiiee/ansible-role-postgres_ssl/

@chukka
Copy link
Collaborator

chukka commented Mar 27, 2025

@bbaassssiiee Thanks ! One more thing - we are planning to move from stable to mainline repo (as default) for nginx to get latest versions for customers as it enables to use new features/fixes , WDYT ? (customers can still change to stable if needed )

mainline : https://nginx.org/packages/mainline/
stable : https://nginx.org/packages/

@bbaassssiiee
Copy link
Contributor Author

@bbaassssiiee Thanks ! One more thing - we are planning to move from stable to mainline repo (as default) for nginx to get latest versions for customers as it enables to use new features/fixes , WDYT ? (customers can still change to stable if needed )

mainline : https://nginx.org/packages/mainline/ stable : https://nginx.org/packages/

That would be the reason for upstream.

@bbaassssiiee bbaassssiiee requested a review from chukka April 3, 2025 07:43
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.

2 participants