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 new dynflow plugin settings #219

Closed
wants to merge 1 commit into from

Conversation

stbenjam
Copy link
Member

@stbenjam stbenjam commented Feb 4, 2016

No description provided.

@@ -2,4 +2,5 @@
class foreman_proxy::plugin::dynflow::params {
$enabled = true
$listen_on = 'https'
$database_path = '/var/spool/foreman-proxy/dynflow/dynflow.sqlite'
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to create this path in both RPM and deb packages first unless it can handle the directory not existing.

It's also worth noting that if this is intended to be persisted indefinitely then it should be /var/lib, and /var/spool should just be used for pending work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, the lack of it existing in the packages is what made me realize it wasn't in the installer, will take care of that too.

I would guess it should be persistent since we could otherwise use the in-memory sqlite.

@iNecas Ok to move this to /var/lib?

Copy link
Member

Choose a reason for hiding this comment

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

Np with me in moving it to /var/lib, @ares objections?

@stbenjam
Copy link
Member Author

stbenjam commented Feb 4, 2016

Path corrected, and here too https://github.com/theforeman/smart_proxy_dynflow/pull/11/files

@@ -17,7 +17,7 @@
:ensure => 'file',
:owner => 'root',
:mode => '0640',
:content => /:enabled: https/
:content => %r{:enabled: https\n:database: /var/lib/foreman-proxy/dynflow/dynflow.sqlite}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe use verify_exact_contents here?

@mmoll
Copy link
Contributor

mmoll commented Feb 4, 2016

👍 otherwise

@mmoll
Copy link
Contributor

mmoll commented Feb 13, 2016

ping?

@stbenjam
Copy link
Member Author

Want theforeman/smart_proxy_dynflow#12 to be merged first so I can include those options too.

@stbenjam stbenjam changed the title Add dynflow database path to config Add new dynflow plugin settings Feb 15, 2016
})

it 'should generate correct dynflow.yml' do
verify_exact_contents(catalogue, "#{etc_dir}/foreman-proxy/settings.d/dynflow.yml", [
Copy link
Member

Choose a reason for hiding this comment

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

etc_dir is not defined here. I wonder if we can do that in our spec_helper so we don't have to do it in every spec file.

Copy link
Contributor

Choose a reason for hiding this comment

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

as the tests are confined to redhat-7-x86_64 at the beginning of the file, the fixed path would be ok

@ekohl
Copy link
Member

ekohl commented Feb 18, 2016

👍

@mmoll mmoll closed this in 5791db1 Feb 18, 2016
@mmoll
Copy link
Contributor

mmoll commented Feb 18, 2016

merged, thanks @stbenjam!

@stbenjam stbenjam deleted the fix-dynflow branch February 19, 2016 01:52
ehelms pushed a commit to ehelms/puppet-foreman_proxy that referenced this pull request Aug 25, 2017
Creates db axis dynamically for test_plugin_matrix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants