Add smart_proxy_chef plugin support #152
Conversation
|
👍 |
|
Plugin classes recently gained "version" and "listen_on" parameters as standard, could you add these too please? |
|
oh, right, I'll add it |
|
@domcleal we should document 'writing a plugin' in the README. |
|
That wouldn't help in this case, I wrote this a long time before I submitted final version with just small changes. I think most people would look to existing plugin in the beginning. Maybe some puppet-lint rule to check whether plugin classes have required parameters would work. |
|
@ares true, but it could help me since I forgot that we wanted to add those. A puppet-lint rule could help but for now manual reviews are the only thing we have. |
ares
force-pushed
the
smartproxy_chef_plugin
branch
from
ddc775d
to
583ac53
Compare
|
@domcleal updated |
| $ssl_pem_file = $::foreman_proxy::plugin::chef::params::ssl_pem_file, | ||
| ) inherits foreman_proxy::plugin::chef::params { | ||
|
|
||
| foreman_proxy::plugin {'chef': } -> |
There was a problem hiding this comment.
You still need to pass on $version here.
ares
force-pushed
the
smartproxy_chef_plugin
branch
from
583ac53
to
9ee2bf5
Compare
|
@ekohl thanks, I also added validations, could you give it another look please? |
ares
force-pushed
the
smartproxy_chef_plugin
branch
from
9ee2bf5
to
0838236
Compare
| foreman_proxy::plugin {'chef': | ||
| version => $version, | ||
| } -> | ||
| foreman_proxy::settings_fule { 'chef': |
|
I think your tests didn't run, as the filename ends in ".spec" rather than "_spec". |
ares
force-pushed
the
smartproxy_chef_plugin
branch
from
0838236
to
bd85a11
Compare
|
Now found found my trick to mask typos :-) Thanks @domcleal very much indeed. Once I get ACK from both of you I'd like to test it end to end again because there were so many changes. After that I can confirm it works and can be merged. |
|
It's very red :) |
| foreman_proxy::plugin {'chef': | ||
| version => $version, | ||
| } -> | ||
| foreman_proxy::settings_file { 'chef': |
There was a problem hiding this comment.
This loads templates/chef.yml.erb while the file is called templates/plugin/chef.yml.erb which leads to red tests.
There was a problem hiding this comment.
Most plugins override it with template_path so I think that would be best here as well.
ares
force-pushed
the
smartproxy_chef_plugin
branch
2 times, most recently
from
ef2f91a
to
ee44bf9
Compare
|
|
||
| it 'should install configuration file' do | ||
| should contain_foreman_proxy__settings_file('chef') | ||
| content = subject.resource('file', '/etc/foreman-proxy/settings.d/chef.yml').send(:parameters)[:content] |
There was a problem hiding this comment.
s/subject/catalogue/ and ditto below
ares
force-pushed
the
smartproxy_chef_plugin
branch
3 times, most recently
from
9972943
to
8ba137c
Compare
|
I made it green again. Also added one helper which is in my opinion better way to test the config file. |
|
👍 |
|
|
||
| it 'should install configuration file' do | ||
| should contain_foreman_proxy__settings_file('chef') | ||
| verify_included_content(catalogue, '/etc/foreman-proxy/settings.d/chef.yml', ':enabled: true') |
There was a problem hiding this comment.
There's already should contain_file('/etc/foreman-proxy/settings.d/chef.yml').with_content(':enabled: true') for this.
|
Not sure if the extra helper adds that much. Can you explain how it differs from |
ares
force-pushed
the
smartproxy_chef_plugin
branch
2 times, most recently
from
19feb90
to
8ba137c
Compare
|
@ekohl the |
|
@ares afaik with_content also accepts a regex: |
ares
force-pushed
the
smartproxy_chef_plugin
branch
from
8ba137c
to
fa087a3
Compare
|
@ekohl good, I didn't know this, I changed it to use |
|
Thanks @ares! Don't forget to send a PR to foreman-installer to add it to the kafo/answer files. |
Initial setup/use of jenkins-job-builder
No description provided.