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

Update the default version of Apache for Amazon Linux 2 #2158

Merged
merged 3 commits into from
Jun 21, 2021

Conversation

turnopil
Copy link
Contributor

  • updated $default variable for Amazon Linux 2

The default httpd package version on the Amazon Linux 2 is 2.4

Name        : httpd
Arch        : x86_64
Version     : 2.4.46
Release     : 1.amzn2
Size        : 1.3 M
Repo        : amzn2-core/2/x86_64
Summary     : Apache HTTP Server
URL         : https://httpd.apache.org/
License     : ASL 2.0
Description : The Apache HTTP Server is a powerful, efficient, and extensible
            : web server.

Based on the above we need to update the $default variable to support AL2

@turnopil turnopil requested a review from a team as a code owner June 16, 2021 14:38
@CLAassistant
Copy link

CLAassistant commented Jun 16, 2021

CLA assistant check
All committers have signed the CLA.

@puppet-community-rangefinder
Copy link

apache::version is a class

Breaking changes to this file WILL impact these 1 modules (exact match):

This module is declared in 175 of 576 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

Copy link
Collaborator

@ekohl ekohl 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 not sure why Ubuntu testing fails but that can't be related.

Maybe another place to check is here:

if $::operatingsystem == 'Amazon' {
$baseurl = 'https://oss-binaries.phusionpassenger.com/yum/passenger/el/6Server/$basearch'
} else {
$baseurl = 'https://oss-binaries.phusionpassenger.com/yum/passenger/el/$releasever/$basearch'
}

AFAIK that is also not correct with Amazon 2 since it's based on EL7 while Amazon 1 was based on EL6.

@turnopil
Copy link
Contributor Author

I'm not sure why Ubuntu testing fails but that can't be related.

Maybe another place to check is here:

if $::operatingsystem == 'Amazon' {
$baseurl = 'https://oss-binaries.phusionpassenger.com/yum/passenger/el/6Server/$basearch'
} else {
$baseurl = 'https://oss-binaries.phusionpassenger.com/yum/passenger/el/$releasever/$basearch'
}

AFAIK that is also not correct with Amazon 2 since it's based on EL7 while Amazon 1 was based on EL6.

I guess the root cause of the Ubuntu failed tests are unmet dependencies:

docker exec litmusimage_ubuntu_16_04-2222 apt-get install -y openssh-server openssh-client
...
Some packages could not be installed.
...
The following packages have unmet dependencies:\n openssh-client : Depends: libedit2 (>= 2.11-20080614) but it is not installable\n
...

Maybe docker image or openssh* stuff has been updated

I'm not familiar with the passenger module. But it has the same problem that I'm trying to fix. Maybe it requires a separate PR?

@turnopil
Copy link
Contributor Author

btw. What I have found in docs: https://github.com/puppetlabs/puppetlabs-apache#rhelcentos-7

RHEL/CentOS 7
The apache::mod::passenger and apache::mod::proxy_html classes are untested because the EL7 repository is missing compatible packages, which also blocks us from testing the apache::vhost defined type's rack_base_uris parameter.

@NapoSky
Copy link

NapoSky commented Jun 17, 2021

Any ideas when this PR will be accepted after merging is cleared ?
Appreciated, thanks!

@ekohl
Copy link
Collaborator

ekohl commented Jun 17, 2021

btw. What I have found in docs: https://github.com/puppetlabs/puppetlabs-apache#rhelcentos-7

That's just testing. However, just from reading the code you can see that it will try to use EL6 repos on Amazon Linux. That should also get a conditional that it's Amazon Linux 1.

Any ideas when this PR will be accepted after merging is cleared ?

If #2158 (review) is addressed.

- added Amazon Linux 2 support for passenger module
manifests/mod/passenger.pp Outdated Show resolved Hide resolved
Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

This looks correct from a patch perspective. I think the modules team can give some advice on the correct commit message so it meets their standards. I'm not sure if we need to squash this etc.

@david22swan
Copy link
Member

Everything looks good from my end so I'll go ahead and merge it :)

@turnopil Thank you for realizing the need for this fix and putting in the work to get it done, hope to see more from you in the future :)

@ekohl Thank you for reviewing this work and helping to get it across the line, great to see you taking advantage of the trusted contributors program :)

Also regarding what you stated in your last message, in regards to the PR titles the general idea is that the title should clearly reflect what the PR is attempting to accomplish so that you can tell at a glance.
In addition as a general rule if the work has a related ticket/issue then the ticket/issue number should be included within the title so as to clearly link them.
When it comes to the amount of commits that are acceptable before a squash is requested, then somewhere between 1-3 is considered to be generally acceptable so long as each commit is clearly distinct.
Though this amount can go up as the size of the overall PR increases, i.e. there are some pieces of work that need to be split up into multiple parts in order for the work done to be better understood.

Sorry if I went on for a while, wanted to be clear.
Once again thank you to the both of you.

@david22swan david22swan merged commit c2e1e6f into puppetlabs:main Jun 21, 2021
@turnopil turnopil deleted the al2 branch June 21, 2021 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants