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

Refactor running with a service to Foreman 1.22 #723

Merged
merged 1 commit into from
Apr 18, 2019

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Apr 3, 2019

This uses the new foreman-service package. It slightly depends on the outcome of theforeman/foreman-packaging#3594.

It needs some parameters to select the port and bind address as well as tests.

@ekohl
Copy link
Member Author

ekohl commented Apr 3, 2019

Note I'm dropping the use of environment files and only use systemd drop ins to be distro agnostic, as is the current best practice. This is breaking and we may need to drop the sysvinit file in Debian packaging. @ehelms @mmoll I'd like some feedback on this.

@@ -339,6 +339,8 @@
timeout => 0,
}

$use_foreman_service = ! $passenger
Copy link
Member

Choose a reason for hiding this comment

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

I get why we need to set passenger to false explicitly in the current setup, but I do find not setting use_foreman_service unobvious when looking at the parameters someone would be specifying. If we will reverse this very soon after the 1.22 release then I can live with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note this is currently not a parameter but rather a variable inside the body so the user can't override this. In the reverse proxy scenario I'm expanding this logic. Because I couldn't properly figure out an API, I opted not to expose this to the user directly.

}

if $::foreman::use_foreman_service {
systemd::dropin_file { 'installer.conf':
Copy link
Member

Choose a reason for hiding this comment

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

Why is it called installer.conf ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it'd make it easy to track where it came from. It ends up as /etc/systemd/system/foreman.service.d/installer.conf

manifests/config.pp Outdated Show resolved Hide resolved
@ekohl ekohl marked this pull request as ready for review April 17, 2019 11:17
@ekohl ekohl force-pushed the foreman-1.22-service branch 6 times, most recently from 02b106e to b7b43fc Compare April 18, 2019 10:30
@ekohl
Copy link
Member Author

ekohl commented Apr 18, 2019

This is now 💚 and even though the Debian side isn't merged (theforeman/foreman-packaging#3643), it will be needed on EL for 1.22 because the service is no longer present unless installed.

@mmoll mmoll merged commit 240b513 into theforeman:master Apr 18, 2019
@mmoll
Copy link
Contributor

mmoll commented Apr 18, 2019

merged, bedankt @ekohl!

@ekohl ekohl deleted the foreman-1.22-service branch April 18, 2019 12:32
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.

4 participants