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 support for nginx_version when compiling NGINX from source #661

Merged
merged 7 commits into from
Oct 3, 2023

Conversation

MohamedLamineAllal
Copy link
Contributor

@MohamedLamineAllal MohamedLamineAllal commented Sep 27, 2023

Note: this is a draft

Proposed changes

Before the change. The latest version is installed. And no ability to fix the version

Checklist

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

  • [ x ] I have read the CONTRIBUTING document.
  • [ x ] 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).

NOTE

  • This is a draft. I didn't run tests. As i was limited in time to setup docker plugin on macos. I implemented the versioning for my own use case.
    (I guess github actions will run the tests)
    • If someone can take the draft, review it and run the tests. It would be nice.
    • Before that change latest version was installed only
  • For the test i added. I'm not sure if it's supported to add nested folder. [✅ Already handled after review ✅]
    • I did copy the version test for from registry install
    • The how the test work is clear
    • I'm assuming nested folders are supported and molecule.yml is how a test is found
    • I'm not familiar with molecule
    • If what i did is not appropriate or doesn't work. The test is good. You can adjust it in the right structure.

@alessfg
Copy link
Collaborator

alessfg commented Sep 28, 2023

At first glance the changes look good!

Molecule does not support nested scenarios (folders) so you'd have to create a new scenario (e.g. source-version) and throw your tests in there. You'll also have to add the new scenario to the GH actions matrix 😁

- moved to source-version folder
- and added to github workflow
As mentioned by this review comment nginxinc#661 (comment)
@MohamedLamineAllal
Copy link
Contributor Author

MohamedLamineAllal commented Sep 28, 2023

At first glance the changes look good!

Molecule does not support nested scenarios (folders) so you'd have to create a new scenario (e.g. source-version) and throw your tests in there. You'll also have to add the new scenario to the GH actions matrix 😁

@alessfg thank you. Changes applied as you suggested.

- adapt the test to use exactly what `source` scenario did use.
- Including the fact that `source` test doesn't skip `oraclelinux-7`
    - # Oracle Linux 7 works in local tests but bugs out in GitHub actions for some undetermined reason
@MohamedLamineAllal
Copy link
Contributor Author

MohamedLamineAllal commented Sep 30, 2023

@alessfg I did update the test, should be good now.

The test did fail with no space left in device. Why is that?
image

  • I wasn't using the exact test scenario as source scenario. Now i switched to use the exact same. With specifying the version as the only difference. And checking for the version (Note as well: the latest now is 1.25.2 and the test does install 1.25.1 which make the test valid for testing version specifying against default latest behavior)

I noticed

platforms: # Oracle Linux 7 works in local tests but bugs out in GitHub actions for some undetermined reason
  • I guess the space thing is related to the bug ?

The old test did include Oracle

  • source scenario does not include it

    • The new update follows source on that
  • I did remove - name: Check if NGINX is installed check i included before. As it's not for when installing from source. I made that mistake before.

@MohamedLamineAllal
Copy link
Contributor Author

MohamedLamineAllal commented Oct 2, 2023

  • All passing except version scenario and only for rhel9
  • The version scenario, doesn't have to do anything with install-source

TASK [nginxinc.nginx : (AlmaLinux/Amazon Linux/CentOS/Oracle Linux/RHEL/Rocky Linux) Install NGINX] ***
fatal: [rhel-9]: FAILED! => {"changed": false, "msg": "Failed to download metadata for repo 'ubi-9-baseos-rpms': Cannot download repomd.xml: Cannot download repodata/repomd.xml: All mirrors were tried", "rc": 1, "results": []}

  • Error related to tasks/opensource/install-redhat.yml
Screenshot 2023-10-02 at 10 31 38 PM Screenshot 2023-10-02 at 10 33 01 PM

@alessfg
Copy link
Collaborator

alessfg commented Oct 3, 2023

This might be due to a temporary issue with an upstream repo, I'll rerun the tests 😄

@alessfg alessfg added the enhancement Enhance/improve an existing feature label Oct 3, 2023
@alessfg alessfg added this to the 0.24.2 milestone Oct 3, 2023
@alessfg alessfg changed the title Add support of nginx_version in instal_from source Add support of nginx_version in install_from source Oct 3, 2023
@alessfg alessfg changed the title Add support of nginx_version in install_from source Add support for nginx_version when compiling NGINX from source Oct 3, 2023
@alessfg alessfg merged commit 70c6ec1 into nginxinc:main Oct 3, 2023
15 checks passed
@alessfg
Copy link
Collaborator

alessfg commented Oct 3, 2023

Merged. Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhance/improve an existing feature
Development

Successfully merging this pull request may close these issues.

its not possible to define nginx_version with nginx_install_from: source to pin the version
2 participants