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

Add 'nginx_app_protect_manage_repo' feature flag and defaults #108

Merged
merged 4 commits into from
Jun 14, 2021

Conversation

sjugge
Copy link
Contributor

@sjugge sjugge commented Jun 11, 2021

Proposed changes

This change adds a nginx_app_protect_manage_repo boolean which can be used to disable Nginx App Protect repo management by this role.

This feature flag addresses edge-cases where Nginx' App Protect repositories are managed by other means and should not be configured by this role in order to install Nginx App Protect. The default behavior of the role is unaltered.

Checklist

  • I have read the CONTRIBUTING document
  • I have added Molecule tests that prove my fix is effective or that my feature works
  • I have checked that any relevant Molecule tests pass after adding my changes
  • I have updated any relevant documentation (defaults/main.yml, README.md and CHANGELOG.md)

Sidenote on Molecule tests, as a test for setting nginx_app_protect_manage_repo to false would effectively require current tests to be disabled, and as the default behavior is not altered, no additional tests have been added.

…ng external Nginx App Protect repo management
@alessfg alessfg added the feature New feature or request label Jun 11, 2021
@alessfg alessfg self-requested a review June 14, 2021 11:25
Copy link
Collaborator

@alessfg alessfg left a comment

Choose a reason for hiding this comment

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

I'm going to copy/paste what I wrote on a similar PR for the NGINX role (nginxinc/ansible-role-nginx#420) if that's fine by you 😝

Minor nitpick, can you rename Nginx to NGINX in comments? Can you update the Changelog too?

Re Molecule tests, you could arguably create a new scenario and setup the NGINX App Protect repo beforehand in the prepare.yml playbook (you can check an example under the plus molecule test and you can probably copy/paste the same tasks found in the role proper to setup the NGINX repository in the prepare.yml playbook). Not necessarily a requirement to merge this PR, but it would be nice.

- update CHANGELOG
- s/Nginx/NGINX/g
@sjugge
Copy link
Contributor Author

sjugge commented Jun 14, 2021

@alessfg dito for nginxinc/ansible-role-nginx#420 ;) , CHANGELOG & NGINX naming updated

@alessfg alessfg added this to the 0.5.1 milestone Jun 14, 2021
@alessfg alessfg changed the title add 'nginx_app_protect_manage_repo' feature flag and defaults Add 'nginx_app_protect_manage_repo' feature flag and defaults Jun 14, 2021
@alessfg alessfg merged commit a926a34 into nginxinc:main Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants