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 CentOS/RedHat yum repository module_hotfixes option #264

Merged
merged 8 commits into from
Jun 18, 2020

Conversation

heronimus
Copy link
Contributor

@heronimus heronimus commented Jun 16, 2020

Proposed changes

Add YUM repository module_hotfixes = True option to protects the repository from package filtering, in case another repository (usually Centos AppStream) is marked as active streams for the Nginx package.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

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

Add 'module_hotfixes = True' option to protects the repository from package filtering, in case other repository (usually Centos AppStream) is marked as active streams for Nginx package.
@alessfg
Copy link
Collaborator

alessfg commented Jun 16, 2020

Thanks for the PR! Could you perhaps add a molecule test too to cover this use case? Modifying the default_centos playbook to include pre_tasks to install NGINX from AppStream, uninstall it, and then run what's already there should work 😄

Add Molecule test pre_task to enable Nginx AppStream module to verify package filtering
@heronimus
Copy link
Contributor Author

Hi @alessfgm,
I just found another fact, this issue only happens with yum with dnf backend (also known as yum4) because of it's modularity feature. So I add pre_task condition only for CentOS/RedHat with major version == 8.
This issue also can be reproduced by only enabling the Nginx @ AppStream package module without installing it first. I can't found ansible dnf resources that equivalent with dnf module enable so I use command for the pre_task. Thanks!

@heronimus
Copy link
Contributor Author

Btw, I really new with this Molecule test. I saw that there's some previous error about the idempotence test, also my committed lineinfile task also doesn't pass the idempotence test. But I think the lineinfile regexp parameter already ensure idempotence. I'm I wrong with this? or is there any alternative lineinfile option to pass the Molecule idempotence test?

@alessfg
Copy link
Collaborator

alessfg commented Jun 16, 2020

What's happening is that the yum_repository module is overwriting the module_hotfixes change done by lineinfile. A potential way around that would be to set changed_when for both yum_respository and module_hotfixes to false, but I'm not a big fan of that. A better solution might be to comment out both tasks until module_hotfixes gets merged to Ansible and released and use something like blockinfile in the meantime to manually create the nginx repo file. Or, alternatively, and perhaps what I'd do, is duplicate both tasks, leave yum_repository as it was before for CentOS/RHEL 6/7, and create a blockinfile for CentOS/RHEL 8.

As far as why the pre_task fails -- command is never idempotent by default. Ansible will always run any command you specify unless you use one of a few workarounds. The easiest solution would be to simply set changed_when to false (it's no big deal seeing as it's a pre-task and worst case, it'll simply not enable the dnf nginx module), but there's a few more involved/better examples online if you want to go the extra mile.

alessfg and others added 5 commits June 17, 2020 14:48
Add changed_when option for dnf shell command
Separate add repository task for CentOS/RHEL 8 with module_hotfixes=true option
Create new file when repo file doesn't exist
@heronimus
Copy link
Contributor Author

Yup got it, I already push some changes for that, also it seems that CI build is passed now, please review again.

Remove unused insertafter properties
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.

LGTM! Thanks for your PR!

@alessfg alessfg merged commit 088a33b into nginxinc:master Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

2 participants