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

Fixes #32947 - Use Apache module variables #968

Merged
merged 2 commits into from Jul 9, 2021

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Jul 6, 2021

Apache is packaged in different locations between Red Hat and Debian. The user differs (apache vs www-data) and conf dir (/etc/httpd vs /etc/apache2). This changes the code to use variables already defined on the apache module to avoid duplicating this logic.

manifests/params.pp Show resolved Hide resolved
@@ -3,7 +3,7 @@
SSLRequireSSL
AuthType GSSAPI
AuthName "GSSAPI Single Sign On Login"
GssapiCredStore keytab:<%= scope.lookupvar('foreman::http_keytab') %>
GssapiCredStore keytab:<%= @http_keytab %>
Copy link
Member

Choose a reason for hiding this comment

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

I was about to say that this should have broken

.with_ssl_content(%r{^\s*GssapiCredStore keytab:/etc/httpd/conf/http.keytab$})

But we don't test this on Debian at all…

context "on #{os}", if: facts[:osfamily] == 'RedHat' do

Mind adding tests? :)

Copy link
Member

Choose a reason for hiding this comment

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

I took the liberty of pushing some simple tests now ;)

ekohl and others added 2 commits July 9, 2021 10:07
Apache is packaged in different locations between Red Hat and Debian.
The user differs (apache vs www-data) and conf dir (/etc/httpd vs
/etc/apache2). This changes the code to use variables already defined on
the apache module to avoid duplicating this logic.
@ekohl
Copy link
Member Author

ekohl commented Jul 9, 2021

I can't approve my own PR, but 👍 and thanks for fixing the tests.

@evgeni evgeni merged commit b759778 into theforeman:master Jul 9, 2021
@ekohl ekohl deleted the 32947-use-apache-vars branch July 9, 2021 09:52
@evgeni
Copy link
Member

evgeni commented Jul 9, 2021

@tbrisker how noteworthy do you find that Debian can now use Kerberos auth too? :)

@ekohl
Copy link
Member Author

ekohl commented Jul 9, 2021

Perhaps we can capture it together in "Kerberos is now available on EL8 and Debian"

@tbrisker
Copy link
Member

Makes sense to add a headline feature regarding the switch to gssapi and its availability on all distros

@tbrisker
Copy link
Member

and now i see you already did that ^_^

@evgeni
Copy link
Member

evgeni commented Jul 12, 2021

Yupp. And now also explicitly added Debian/Ubuntu in theforeman/theforeman.org#1858

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants