search

nginxinc / ansible-role-nginx

Ansible role for installing NGINX
https://galaxy.ansible.com/nginxinc/nginx
Apache License 2.0
644 stars 348 forks source link

Amazon Linux 2023 support #597

Closed sbezugliy closed 7 months ago

sbezugliy commented 1 year ago

Proposed changes

Amazon Linux 2023 is not supported by current Ansible role. Also it has some significant differences with Amazon Linux 1, 2, so same actions aren't compatible. List of differences and issues:

Checklist

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

thresheek commented 1 year ago

I think for the versioning/numbering issue you can just use priority=9 for nginx.org repos to be preferred without disabling the amazon own repo.

alessfg commented 1 year ago

Thanks for the PR! I'll get to it asap!

Two initial thoughts before I actually do a review:

a) Looks like you ran ansible-lint --write? I might ask you to revert some of the changes the tool does by default. The changes it does are sometimes not that great for readibility b) Looks like some tests are failing? I had a quick look and all Molecule distro specs are identical across all scenarios, so I am not entirely sure why it sometimes fails? A simple rerun might be good enough to fix those errors, but if they keep failing we might need to look into it a bit more.

sbezugliy commented 1 year ago

Yes, sure I'll recommit linted code without auto fixes. I'm trying to understand what happens with Molecula tests... But in other parts works well at my deployment: Nginx installed, configs looks well, all checks passed and server started up.

So, I'll adjust this PR soon. Thank you

alessfg commented 1 year ago

Ok some of these tests are now failing because a new version of NGINX got released over night and it broke some of the Molecule verification tests. I'll open a PR with a fix for that either later today or tomorrow and then we can try to run these tests again :)

alessfg commented 1 year ago

Thanks for the updates!

  1. Looks like some Molecule scenarios are still missing 😄 (e.g. there is no AL2023 platform in the default scenario). Also, if you could comment out/remove the platform parameter (e.g. https://github.com/nginxinc/ansible-role-nginx/pull/597/files#diff-f653be9e6e7813c18a248edeb270c6a8034e1ed3b0ea3e49e6e6a9192f740201R65) for the time being, that'd be great. The pipeline has been running into some emulation related issues.
  2. The linter is failing -- not entirely sure why but there might be some issues with the YAML code you've written.
sbezugliy commented 1 year ago

Molecule is new for me, but I know what to do with it. So, trying... Yes, I'll check one more time syntax and fix it next. Looks like it's typo with quotes.

alessfg commented 7 months ago

Closing this PR in favor of #706.

github-actions[bot] commented 7 months ago

CLA Assistant Lite bot: 🎉 Thank you for your contribution. It appears you have not yet signed the F5 Contributor License Agreement (CLA), which is required for your changes to be incorporated into an F5 project. Please kindly read the F5 CLA and comment the following to agree:

I have hereby read the F5 CLA and agree to its terms

You can retrigger this bot by commenting recheck in this Pull Request