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

Use foreman apache configs to unblock Pulp 3 #225

Merged
merged 1 commit into from
Feb 19, 2020

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Feb 14, 2020

This module had declared it's own vhost using a similar naming convention and directory structure as Foreman's. This causes a conflict when pulpcore is enabled since pulpcore relies on adding fragments to Foreman's vhost declaration specifically around the use of 05-foreman-ssl.d. To solve this, and bring greater continuity I have attempted to declare the foreman::config::apache class and re-use it here.

manifests/apache.pp Outdated Show resolved Hide resolved
@ehelms
Copy link
Member Author

ehelms commented Feb 14, 2020

@ekohl general thoughts on this overall approach? It is related to this change theforeman/forklift#1105

Copy link
Member

@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.

In my local dev env I have a WIP patch that attempted to do the same thing but it looks like I never finished that. It was certainly my intention to use it this way. I also thought about refactoring the class slightly in puppet-foreman to make it easier to consume. We can do that by not having defaults inheriting from foreman but rather working defaults. Then in the foreman::config class we explicitly set the parameters.

manifests/apache.pp Outdated Show resolved Hide resolved
manifests/apache.pp Outdated Show resolved Hide resolved
manifests/apache.pp Outdated Show resolved Hide resolved
manifests/apache.pp Outdated Show resolved Hide resolved
@ekohl
Copy link
Member

ekohl commented Feb 14, 2020

I'd like to see a version that uses theforeman/puppet-foreman#800 because it should be simpler.

@ehelms ehelms force-pushed the use-foreman-apache branch 2 times, most recently from 56b9b5e to 3aba7cf Compare February 19, 2020 02:30
manifests/apache.pp Outdated Show resolved Hide resolved
manifests/apache.pp Outdated Show resolved Hide resolved
manifests/apache.pp Show resolved Hide resolved
manifests/init.pp Outdated Show resolved Hide resolved
manifests/init.pp Outdated Show resolved Hide resolved
@ehelms
Copy link
Member Author

ehelms commented Feb 19, 2020

All cleaned up and ready to party

Copy link
Member

@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.

IMHO it makes sense to further update the module in later PRs to isolate more parts

@@ -178,37 +178,9 @@
require => Class['katello_devel::database'],
}

User<|title == $user|>{groups +> 'qpidd'}

# TODO: Use katello::pulp
Copy link
Member

Choose a reason for hiding this comment

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

Yay for cleaning up my TODOs

@ekohl ekohl merged commit 254df33 into theforeman:master Feb 19, 2020
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.

3 participants